- User Since
- Apr 10 2020, 4:29 PM (62 w, 7 h)
Fri, Jun 4
since I cannot repro it locally, let's have this patch in to resolve -EHa -O2 crashes for now.
I will add more -O2 tests in following patches.
@zahiraam, are you removing all those CHECKs:
Thu, Jun 3
@aganea Oh, my mistake. did not mean to enable -fasync-exceptions under -EHa in this patch. will fix it shortly...
Hi, yes I can repro the crash with -fasync-exceptions plus -O2 option. Working on it now..
thank you for reporting this bug..
@aganea , this patch should be zero-impact without explicit option -fasync-exceptions. Are you also seeing a crash without this option?
Tue, Jun 1
Thank you for reporting this . From the callstack it does not seem related to this patch.
Could you share the repro case @__compile.rsp temp.i with me?
Mon, May 24
added RUN command in one test.
May 19 2021
May 17 2021
May 16 2021
Passed many CodeGen related test suites over the weekend.
It landed in..
Apr 14 2021
bool keepFramePointer(const MachineFunction &MF) const override;
Apr 12 2021
Hi, (Last call for review!!)
Is there any more comments? This review has lasted for more than a year now. I believe I had addressed or answered all questions and concerns. Thank all reviewers' precious feedbacks.
For late comers, again
(1) Original llvm-dev [RFC] discussions can be found in these two threads below:
Apr 8 2021
fixed command option typos in two test cases
Apr 7 2021
Apr 5 2021
hi, I believe I'd addressed all issues or concerns, and it's rebased to up-to-date source in new _main branch now. Does this look good to everyone? If I don't hear any objection in a couple of days, I will go ahead make this patch in. Again This is just part-1 of Windows SEH feature. Without new option specified, it's a zero-impact change.
Apr 3 2021
rebase the patch
Apr 1 2021
Removed some files (mostly for Part 2 of this feature) that were accidentally put in last revision.
Feb 23 2021
changed option name and a couple of minor changes per John McCall's comments.
Feb 17 2021
thank you for the thorough review again. My answer for each comment below:
Jan 28 2021
sorry for the delay. I'm still in the middle of something.
will context-switch a little bit latter. thanks,
Dec 17 2020
Hi, Is there any more concerns, feedbacks or suggestions?
Nov 24 2020
Nov 19 2020
Per Joseph's feedback, further clarify the semantic of llvm.seh.try.begin and llvm.seh.try.end.
Fixed the clang fault when built without '-feh-asynch' option.
EHStack's CGF field must be initialized even without -feh-asynch.
Nov 18 2020
per review feedback from John McCall and others, this update includes:
Sep 28 2020
Summarize our discussion above, I will follow up with:
Sep 24 2020
Oh, yes you are right. I was talking about reordering between inlined faulty instructions is allowed. Reordering inlined instruction with any 'direct' volatile instruction should be prohibited. I overlooked LLVM doesn't promise not to reorder volatile and non-volatile. I will simply disable inlining into a _try scope. thanks!
Sep 23 2020
OK, good to know this. thank you!
Other than blocking some opts, does this returns_twice attribute have some other implications?
Sep 22 2020
Oh, sorry I misread it.
For HW exceptions the 'implicit' edge is unavoidable unless an explicit approach (like previous iload/istore) is employed. iload approach was extensively discussed and evaluated when it's proposed. As far as I know, the concerns were that it could create many Invokes, complicate flow graph and possibly result in negative performance impact for downstream optimization and code generation. Making all optimizations be aware of the new semantic is also substantial.
So this design applies an IMPLICT approach. I respectfully disagree that it's problematic because as long as volatile attribute is honored it's robust. please see the C & C++ SEH rule stated in patch description section.
Sep 21 2020
Thank you for prompt reply again.
Sep 17 2020
Hi, John, thank you for reviewing this patch and providing feedback.
regarding your comments:
Jul 12 2020
Jun 11 2020
this patch has lasted for a couple of months. a bug in this area is hard and time-consuming to diagnose.
it's better to get this fix in sooner than later. could someone review and approve it?
does it look good to you? or any more comment?
Jun 4 2020
update a couple of changes per David Majnemer's suggestion.
Jun 3 2020
thank you David. will update and submit a new patch shortly.
Jun 2 2020
Is there any feedback? thanks,
Hi, does this look good? or is there any other concern?
May 26 2020
update LangRef.rst for new intrinsics
May 22 2020
fixed tidy warnings
It may be helpful (even for the reviewers) to first specify their behavior,
instead of writing that after-the-fact "backwardly" based on the implementation.
Per Roman Lebedev's feedback, divide the patch into Clang and LLVM.
avoid lint warnings and lint hang on Windows
May 21 2020
May 20 2020
May 15 2020
May 13 2020
Hi, Is there more concern?
To re-iterate the implementation strategy of this change:
right, I noticed that too and looked into it further..
Per discussion in https://reviews.llvm.org/D79760 , x86 test case there failed.
I meant did you debug into the crash of your test case in https://bugs.llvm.org/show_bug.cgi?id=42266 ?
May 12 2020
Did you get to the root of the bug and check the correctness of ctor&dtor ordering on your test cases?
Or you just tried to copy MSVC?
Is this the same bug as this patch, https://reviews.llvm.org/D79474/new/ , right?
May 6 2020
thank you David, good eyes!
Is there any more concern?
fixed a comment.
the existing test case is fixed. I think there is no need to add one more unit test for this patch.
fix a typo in test case
May 3 2020
May 1 2020
I found you are the Code Owner of "Exception handling, Windows CodeGen, ARM EABI".
could you please provide a quick review here? thanks,
Apr 28 2020
any more concern or comment?
Apr 16 2020
right, thank you Reid. I see it now :
remade a patch after re-sync
Apr 15 2020
It can be greater than 2 because this Map includes Decls of User locals from parent.
see CodeGenFunction::EmitCapturedLocals() (the same place of this fix).
.. auto I = ParentCGF.LocalDeclMap.find(VD); if (I == ParentCGF.LocalDeclMap.end()) continue;
Replace F_HasSehAbnormalExits with F_HasExitSwitch
The fix there deals with SEH filter with SEH _finally parent where its prototype is FIXED (2 implicit parameters). It will never change.
Apr 14 2020
Do not use name comparison to locate parent's alloca of frame-pointer-addr.
search parent's LocalDeclMap instead.
Apr 13 2020
Remove hard-code name "frame-pointer".
get the name from 2nd Arg of the _finally().
Apr 12 2020
Per Eli's suggestion,
Use icmp (and zext) to check the JumpDest index, instead of directly passing the Index to _finally().
Fixed the format at comment lines
Add option -fms-extensions in test case
Apr 11 2020
Per Eli's feedbacks:
(1) a test case (windows-seh-abnormal-exit.c) is added under clang\test\CodeGen directory.
(2) an assert is added to catch the extremely rare case that the number of JumpDests in a function is greater than 255. The max. number of JumpDests is 1 (return) + 2*M + N, where M is the number of for/while-loops and N is the number of Goto targets.