- User Since
- Apr 10 2020, 4:29 PM (116 w, 4 d)
May 18 2022
What is the current status of this fix?
May 9 2022
This isn't really the distinction I'm trying making here. The distinction here is between LLVM IR uses of blockaddress (for indirectbr), and MachineFunction uses of MachineBasicBlock labels (for retpoline etc.). Jump tables would fall into the latter category, except that we have explicit code to update jump tables in the places where it would matter.
May 6 2022
How about AddressTaken_Internal and AddressTaken_External?
the _Internal is used for blockAddress-constant, jump table, etc where it's targeted by IR internal user code domain. the latter indicates that the address is taken and can be targeted externally by Runtime (EH case), OS or even HW (like the retpoline case).
May 4 2022
May 3 2022
May 2 2022
Apr 29 2022
Apr 28 2022
Apr 25 2022
Apr 22 2022
Apr 18 2022
Apr 14 2022
Dec 9 2021
Nov 29 2021
Hi Joseph, all good points. see replies below.
Nov 11 2021
I greatly appreciate your thorough review and insightful feedbacks.
new revision is provided. also review it again.
Update per Joseph Tremoulet's feedback.
Oct 2 2021
Thank Joseph's in-depth code review and many great suggestions. will work on it and reply back later..
Sep 22 2021
The previous did get lots of substantial and quality reviews (especially those from rjmccall).
Roman, you asked about LLVM design & implementation when reviewing part-1. Here you go; Do you have any feedback?
Does anyone have any more feedbacks?
We would like to land this patch in shortly.
Aug 15 2021
Jun 4 2021
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:
Jun 3 2021
@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?
Jun 1 2021
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?
May 24 2021
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?