- User Since
- Jan 15 2018, 8:31 AM (280 w, 2 d)
LGTM; I always have to re-learn what the combinations of uwtable and nounwind mean, but this seems reasonable to me
Tue, May 23
Mon, May 22
LGTM, thank you again for attempting the switch to doing this unconditionally! I was clearly wrong about there being no dependence if even an in-tree project is breaking
Wed, May 17
(Cross-posting from the other thread, to keep everything in one place)
Tue, May 16
I took a stab at a patch to address the assertion at D150713
Thu, May 11
Does GCC have the same -ftime-trace= option? It seems like it doesn't, as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92396 is still open?
Tue, May 9
Mon, May 8
Apr 27 2023
I am OK to give LGTM, assuming the other reviewers don't still have reservations?
Apr 26 2023
Apr 25 2023
I'm a bit confused after trying to work out the rules for the GCC version of -dumpdir over at https://gcc.gnu.org/onlinedocs/gcc/Overall-Options.html#index-dumpdir but it at least seems like our version is a subset of theirs.
LGTM, thank you!
Apr 24 2023
Apr 19 2023
Now that we are no longer contrained by constexpr, just use
SmallVector for everything.
Determined constexpr was not useful here
Apr 18 2023
LGTM with 1 small nit
Apr 17 2023
LGTM, thank you!
Apr 13 2023
OK, I may have just been overly concerned about this; my familiarity with the X86 target in LLVM is very limited.
I definitely understand the complexity in DwarfExpression::addMachineRegExpression makes any change very difficult. I am happy with continuing to apply the offset to the CFA in the frame base, and if some other brave soul wants to change that in a future patch they won't be any worse off.
Yeah I don't really want to do that here but I agree that folding the offset between the CFA and the stack pointer into the variable expressions rather than leaving it in the DW_AT_frame_base expression is the ideal end point.
My current leaning is to change to using the CFA unconditionally, but offset to maintain the same frame base value. It will result in a slightly larger expression in the default case, but I would be much more confident that the result is always correct.
So, to be clear, you're learning towards using a CFA-based DW_AT_frame_base in all three of these scenarios?
- There is a CFA, and LLVM is currently using %rsp as the DW_AT_frame_base, and %rsp changes throughout the function.
- There is a CFA, and LLVM is currently using %rsp as the DW_AT_frame_base, and %rsp remains constant (after the prologue).
- There is a CFA, and LLVM is currently using %rbp as the DW_AT_frame_base.
Where using the CFA for 1 is what this patch as originally proposed does, and 1 + 2 is the "middle ground" you suggested previously, as I understand it.
I think using the CFA for 1 + 2 + 3 makes more sense than doing just 1 + 2, so I think we more or less agree here.
Apr 12 2023
Very reasonable, but if GCC is already using the CFA unconditionally it seems likely that implies a pretty good support base?
- If the frame pointer is present (or the stack pointer remains unchanged) using it works today. Don't fix what isn't broke.
I definitely think this holds a lot of weight, but just having one concept (CFA) instead of several (stack-pointer, frame-pointer, frame-base, CFA) is also very attractive. The fact that the patch is even necessary also demonstrates something is broken, although it might be the exception that proves the rule.
- If there are any bugs out there that result in an incorrect CFA the machine registers are more likely to be correct as they're actually used by the program and thus issues are more likely to be noticed.
If the CFA is incorrect, unwinding seems like it is doomed to fail, which should constitute a program execution bug if e.g. exceptions are enabled, right? I don't necessarily think we will end up with more elusive debug-info bugs as a result of relying on the CFA.
Although it seems the gcc maintainers were not persuaded by any of this. gcc emits DW_OP_call_frame_cfa in every circumstance I can think of (at least on x86-64).
- If we cannot use it unconditionally, can the condition be more direct, i.e. can we remove the heuristic?
I'm not aware of any reason we cannot use the CFA unconditionally (at least on x86) if it is present. I believe it is possible to remove the setHasCFIAdjustCfa heuristic and use the CFA in place of the stack pointer unconditionally if the above arguments are not persuasive. I believe it's also possible to use the CFA in place of the frame pointer even when it's present, if desired.
I meant to propose another middle-ground, where we use the CFA whenever !hasFP, which (IIUC) is a precondition for x86 ever adjusting the CFA. It would mean we use the CFA more than with the heuristic, but still not unconditionally.
Apr 11 2023
Thank you for the patch! Sorry for the long delay in review
Apr 6 2023
Apr 5 2023
Mar 31 2023
I've also submitted a proposal for a standard version of the operation (thread at https://lists.dwarfstd.org/pipermail/dwarf-discuss/2023-March/002199.html)
Mar 30 2023
Mar 21 2023
Mar 20 2023
Mar 15 2023
It might be worth including the rationale in the description/commit message, like you've done in other similar changes. I assume it is a performance improvement?
Mar 14 2023
Mar 13 2023
Is there a corresponding document update to go along with this? If consumers of the bundles will begin to rely on this it should be documented that it is guaranteed by the producer.
Mar 6 2023
- Fix test name to reflect it testing both r600 and amdcn
- Common up some checks between r600 and amdgcn
- Adjust some whitespace to make test more readable
Mar 1 2023
Feb 27 2023
Feb 21 2023
Seems reasonable to me! LGTM with a small nit
Feb 20 2023
Feb 17 2023
Feb 16 2023
This is great, I also didn't realize there were such a thing :)
Feb 15 2023
Feb 9 2023
Sorry for the delay in reviewing! LGTM with a couple nits
Feb 2 2023
Are there any thoughts on whether this is too ugly to live? It will be awkward to teach users the current default behavior without this change, but if we can accept it as a historical quirk that may be OK.
- Drop [Clang] from commit message.
- ltocgo -> ltoCgo
Feb 1 2023
Replace asserts added in previous version of patch with true diagnostic errors.
The only excpetion is COFF, where the parsing still deals in integral values
and ensures they fall in the right range, which the Driver assumes/asserts.