This is an archive of the discontinued LLVM Phabricator instance.

[Clang][MC][MachO]Only emits compact-unwind format for "canonical" personality symbols. For the rest, use DWARFs.
ClosedPublic

Authored by oontvoo on Feb 28 2023, 12:23 PM.

Details

Summary

Details: https://github.com/rust-lang/rust/issues/102754

The MachO format uses 2 bits to encode these personality funtions, with 0 reserved for "no-personality".
This means we can only have up to 3 personality. There are already three popular personalities: gxx_personality_v0, gcc_personality_v0, and __objc_personality_v0.
As a result, any system that needs custom-personality will run into a problem (case in point : Rust).

This patch implemented jyknight's proposal to simply force DWARFs for all non-canonical personality functions , ie., __gxx_personality_v0 and __objc_personality_v0 (intentionally left out __gcc_personality_v0 since it's rarely used anyway - there is no point in reserving a spot for it.)

Additionally, added a flag to let users request emitting compact-unwind for non-canonical personality functions if they know it'd be OK for their builds (We have one "free" slot left).

Diff Detail

Event Timeline

oontvoo created this revision.Feb 28 2023, 12:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2023, 12:23 PM
oontvoo requested review of this revision.Feb 28 2023, 12:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2023, 12:23 PM

P.S: Will add tests/whatnot. This is just a draft proposal to get a feel for how folks feel about this change. Thanks

jyknight added inline comments.Feb 28 2023, 12:55 PM
llvm/include/llvm/MC/MCDwarf.h
701 ↗(On Diff #501261)

This is an Apple compact-unwind-specific predicate, so needs to be named as such.

Although, I think with the other change I suggest, we don't need to add a bool at all, we can just check as needed in generateCompactUnwindEncoding.

llvm/lib/MC/MCDwarf.cpp
1867 ↗(On Diff #501261)

I think the check for whether the function is a canonical personality-function should be done inside each architecture's generateCompactUnwindEncoding (which means passing FI.Personality to it from generateCompactUnwindEncodings). I'd stick the check right after the initial "if(X) return 0" clauses.

That would allow the rest of the changes in this file to be reverted.

int3 added a comment.Feb 28 2023, 1:09 PM

Why not let the end user decide whether to enable DWARF unwind by passing -femit-dwarf-unwind?

As a result, any system that needs custom-personality will run into a problem.

This isn't true; it will only run into a problem if it is using all 3 of the canonical personalities, which isn't always the case. E.g. we definitely link Rust programs successfully even without this

So, sigh -- I've just realized this will need a (small) linker change, as well. Currently, we (both LLD and LD64) encode the personality function in the compact-unwind even for unwind infos marked MODE_DWARF.

I believe this is unnecessary, however: in libunwind, getInfoFromCompactEncodingSection is called first, and if it says "use dwarf", then it calls getInfoFromDwarfSection which calls getInfoFromFdeCie, which will overwrite _info.handler with the personality retrieved from the DWARF CIE. (Though, I haven't yet verified it functions as I believe it does with a real-world test).

To fix this, the linker should skip encoding a personality for the compact-unwind entry when the compact-unwind is MODE_DWARF. In lld, that's probably most easily done by setting cu.personality = nullptr instead of fde.personality in UnwindInfoSectionImpl::relocateCompactUnwind. In ld64, I dunno, but might be easier to implement by just skipping such DWARF entries in the pass that assigns the personality-indices.

Why not let the end user decide whether to enable DWARF unwind by passing -femit-dwarf-unwind?

One issue is that we need to emit ONLY dwarf-unwind for those functions which use nonstandard personalities, while femit-dwarf-unwind enables emitting both compact-unwind and dwarf unwind.
Another issue is that this is an assembler flag, and especially in LTO mode, we would not want to use dwarf unwind on ALL functions, only those using a non-standard personality.

As a result, any system that needs custom-personality will run into a problem.

This isn't true; it will only run into a problem if it is using all 3 of the canonical personalities, which isn't always the case. E.g. we definitely link Rust programs successfully even without this

Yes, but getting an error when you add an additional object to the link is not a good experience. The default behavior should be to produce reliably-linkable code. But I think it'd be OK to add an LLVM MC option -compact-unwind-all-personalities as an 'expert' option, if you somehow know that in your project, you won't overflow personalities.

int3 added a comment.Mar 1 2023, 7:46 PM

One issue is that we need to emit ONLY dwarf-unwind for those functions which use nonstandard personalities, while femit-dwarf-unwind enables emitting both compact-unwind and dwarf unwind.

So IMO the ideal design is that we emit both and have the linker fall back to DWARF unwind info if the number of personalities is too large.

Another issue is that this is an assembler flag, and especially in LTO mode, we would not want to use dwarf unwind on ALL functions, only those using a non-standard personality.

Hm, how does LTO mode make a difference?

Anyway if we go with the abovementioned idea of having the linker decide which encoding to use, then emitting both at compile time doesn't seem like a huge problem (+ was the default behavior until recently).

Yes, but getting an error when you add an additional object to the link is not a good experience. The default behavior should be to produce reliably-linkable code. But I think it'd be OK to add an LLVM MC option -compact-unwind-all-personalities as an 'expert' option, if you somehow know that in your project, you won't overflow personalities.

Agreed that we shouldn't throw surprising linker errors, but I don't think making all but 3 languages second-class citizens is a good solution either. Having the linker handle multiple languages gracefully seems better.

oontvoo planned changes to this revision.Mar 2 2023, 10:37 AM

One issue is that we need to emit ONLY dwarf-unwind for those functions which use nonstandard personalities, while femit-dwarf-unwind enables emitting both compact-unwind and dwarf unwind.

So IMO the ideal design is that we emit both and have the linker fall back to DWARF unwind info if the number of personalities is too large.

OK - this sounds like a reasonable behaviour. 👍

One issue is that we need to emit ONLY dwarf-unwind for those functions which use nonstandard personalities, while femit-dwarf-unwind enables emitting both compact-unwind and dwarf unwind.

So IMO the ideal design is that we emit both and have the linker fall back to DWARF unwind info if the number of personalities is too large.

OK - this sounds like a reasonable behaviour. 👍

+1.

I initially had expected that we would not need any linker changes if the compiler emitted only compact-unwind. However, since we DO need to make a linker change in any case...might as well do this slightly more complex change, in order to get the better result.

The linker could choose the 3 compact-unwind personalities slots as follows:

  1. First, choose all personalities which are used by functions having compact-unwind without a DWARF fallback. If there's >3 of these, error.
  2. After that, sort personality fns by how often they're used, and fill in the remaining slots with the most popular functions.

(This does mean we need to preserve the eh_frame info for all functions even when they have compact-unwind too -- right now both are stored in the same Defined::unwindEntry field, since we only ever care about eh-frame unwind if the fn doesn't have compact-unwind)

For the assembler-side: I propose the default behavior ought to be:

  1. For __gxx_personality_v0 and __objc_personality_v0 (but not __gcc_personality_v0), emit only compact unwind (or only DWARF where compact-unwind isn't possible) for each function -- not both. (This saves object-file size/build time for the most common languages).
  2. For all other personalities, emit both compact-unwind+DWARF, so the linker can choose per above.
int3 added a comment.Mar 2 2023, 1:32 PM

For gxx_personality_v0 and objc_personality_v0 (but not __gcc_personality_v0), emit only compact unwind (or only DWARF where compact-unwind isn't possible) for each function -- not both. (This saves object-file size/build time for the most common languages).

Does Swift use a personality function? I'm concerned that we may be de-optimizing a very common use case. However (from looking a small test program) it seems that maybe it doesn't...

int3 added a comment.EditedMar 2 2023, 2:05 PM

I'm also curious about the motivation here; this wouldn't be the most trivial linker change, especially if we are trying not to regress perf. Moreover, ld64 doesn't do this; I'm not even sure it has any flags that let it support more than 3 personalities. Is it just a linker UX thing?

I'm also curious about the motivation here; this wouldn't be the most trivial linker change, especially if we are trying not to regress perf. Moreover, ld64 doesn't do this; I'm not even sure it has any flags that let it support more than 3 personalities. Is it just a linker UX thing?

The motivation is to support programs that use more than 3 personalities (which, unfortunately, is not very rare in our case - and apparently not very rare in the open, either - see the Rust bug attached in the description).
It's true that LD64 doesn't allow it - but that seems like an artificial limitation - there's no real reason we cannot make it work.

int3 added a comment.Mar 3 2023, 9:45 AM

Oops, I'd glossed over that in the commit message. (Also, wow, that is a *long* bug discussion.)

Now that I see it's explicitly driven by Rust needs, I guess my worries about making it a "second-class citizen" are unfounded :)

And if my Swift test case is representative, I guess Swift doesn't actually need a personality function then we don't have to worry about that either. I would like us to be certain of that before proceeding though...

(Though, I haven't yet verified it functions as I believe it does with a real-world test).

Verifying this would help clarify the path forward too :) But assuming that it works:

For gxx_personality_v0 and objc_personality_v0 (but not __gcc_personality_v0), emit only compact unwind (or only DWARF where compact-unwind isn't possible) for each function -- not both. (This saves object-file size/build time for the most common languages).

This seems reasonable.

(This does mean we need to preserve the eh_frame info for all functions even when they have compact-unwind too -- right now both are stored in the same Defined::unwindEntry field, since we only ever care about eh-frame unwind if the fn doesn't have compact-unwind)

Now that you've pointed this out, I'm starting to balk at the added complexity (I know, even though I proposed the idea.) We would have to ensure that everything that accesses unwindEntry (in particular dead-stripping and ICF) now handles both cases correctly. This work will often be redundant too (since only one of the two copies is really needed.) We would also be increasing the size of class Defined, which is not ideal.

As I understand, ld64's best workaround for this right now is the -no_compact_unwind flag. This would be trivial for us to support but of course not ideal from a binary size standpoint. However, from a UX standpoint, "getting an error when you add an additional object to the link" is basically the current situation. I think it is more important for us to prioritize perf + implementation simplicity over a somewhat rare UX edge case...

So my proposal is to have a --prefer-dwarf-unwind flag. If both compact and DWARF unwind are available in the object file, this flag will tell LLD to prefer DWARF instead of compact unwind. Coupled with the abovementioned change to the assembler, this will get us compact unwind when compiling for "canonical" languages while giving us DWARF unwind for the others, while not requiring us to simultaneously track both encodings in the linker.

As I understand, ld64's best workaround for this right now is the -no_compact_unwind flag. This would be trivial for us to support but of course not ideal from a binary size standpoint. However, from a UX standpoint, "getting an error when you add an additional object to the link" is basically the current situation. I think it is more important for us to prioritize perf + implementation simplicity over a somewhat rare UX edge case...

+1 Agreed that impl simplicity should be prioritized.

So my proposal is to have a --prefer-dwarf-unwind flag. If both compact and DWARF unwind are available in the object file, this flag will tell LLD to prefer DWARF instead of compact unwind. Coupled with the abovementioned change to the assembler, this will get us compact unwind when compiling for "canonical" languages while giving us DWARF unwind for the others, while not requiring us to simultaneously track both encodings in the linker.

I've slept on this a bit and I'm not sure how this flag would preserve performance while ALSO simplify the implementation.

  1. If this is set to true, then the linker will end up using only CU formats for C++ and ObjC, but DWARFs for others. (ie., presumably, this will be set when there are 4+ personalities).
  2. If this is false, then the linker will try to use all CUs (and only fallback to DWARF when CU is not present).

So in other words for case #1, we'd end up with one "unused" CU slot. (Because, if I understand it correctly, you're proposing this flag to avoid the complexity of deciding which personality gets the free slot).

But now that you've seen the full picture, I wonder if we could take a step back and re-consider the original proposal (maybe with some modification):
MC changes:

  • For the two ObjC and C++ personalities, always emits compact-unwinds (unless DWARF is explicitly required)
  • For the rest, always emit DWARF (unless compact-unwinds is explicitly required, maybe via something new flag that's default to off)

Linker changes:

  • For the two ObjC and C++ personalities, always assume CUs (unless it's not present then fallback to dwarf - this is not a change in behaviour)
  • For the rest, always use DWARF (unless CU is requested for a specific personality specified via a new flag).

Would this be a bit simpler?

Pros:

  • We don't increase too much binary size (ie., compared to the proposal where we emits both CUs and DWARF)
  • Can still preserve the performance when possible

Cons:

  • Need two new flags (1 in the assembler and 1 in the linker)
int3 added a comment.Mar 6 2023, 1:56 PM

So in other words for case #1, we'd end up with one "unused" CU slot.

Not sure why you think so -- we have three canonical personalities, not two, right? __gcc_personality_v0, __gxx_personality_v0, and __objc_personality_v0. Why would we end up with an unused slot?

+1 Agreed that impl simplicity should be prioritized.

With a goal of simplicity, I think we should just go back to the earlier proposal: Clang emits all functions with a personality other than the 3 canonical personalities as DWARF unwind ONLY. Agreed we can have a flag to opt-into compact unwind here, if a user knows it'll be ok in their build.

And hopefully with the linker fix proposed in an earlier comment to not waste the compact-unwind personality space on DWARF unwind functions, we should be good to go:

To fix this, the linker should skip encoding a personality for the compact-unwind entry when the compact-unwind is MODE_DWARF. In lld, that's probably most easily done by setting cu.personality = nullptr instead of fde.personality in UnwindInfoSectionImpl::relocateCompactUnwind.

It shouldn't need a new linker flag.

int3 added a comment.Mar 6 2023, 4:41 PM

With a goal of simplicity, I think we should just go back to the earlier proposal: Clang emits all functions with a personality other than the 3 canonical personalities as DWARF unwind ONLY. Agreed we can have a flag to opt-into compact unwind here, if a user knows it'll be ok in their build.

Okay yeah this sounds good. The advantage it has over my --prefer-dwarf-unwind proposal is that it will work by default + does not require a new linker flag.

And if my Swift test case is representative, I guess Swift doesn't actually need a personality function then we don't have to worry about that either.

It has been pointed out to me that Swift "exceptions" are just syntax sugar over return statements, so no unwind / personality functions are involved.

Thanks for taking the time to hash things out!

oontvoo updated this revision to Diff 503108.Mar 7 2023, 11:19 AM
oontvoo marked 2 inline comments as done.
  • reverted to original proposal with some modification (only make objc and c++ personality symbols canonical - leaving out gcc)
  • addressed review comments

So in other words for case #1, we'd end up with one "unused" CU slot.

Not sure why you think so -- we have three canonical personalities, not two, right? __gcc_personality_v0, __gxx_personality_v0, and __objc_personality_v0. Why would we end up with an unused slot?

James' previous comment was suggesting to leave out __gcc_personality_v0 from the set of canonical (because it's rarely used), leaving one unused slot.

But yeah, I agreed with going back to the original proposal ;)

oontvoo planned changes to this revision.Mar 7 2023, 12:48 PM

Adding an optional flag to request compact-unwind for non-canonical personalities.

oontvoo updated this revision to Diff 505593.Mar 15 2023, 12:37 PM

New change: added a flag (default to OFF) to allow emit compact-unwinds for non-canonical personalities.

Looks reasonable to me -- but I'd like to see the LLD change implemented and both pieces tested together end-to-end before this is submitted.

James' previous comment was suggesting to leave out __gcc_personality_v0 from the set of canonical (because it's rarely used), leaving one unused slot.

My suggestion was primarily for the previous proposal. Excluding it doesn't really have much of a benefit currently, since we also won't use it for anything else.

Probably fine to leave as you have it though, since C unwind is so rarely used anyways. And in the potential future where we decide to implement the more complex scheme of choosing whether to use compact or dwarf unwind at link-time, not having prebuilt objects/archives hanging around that require all 3 slots of compact-unwind would be helpful.

llvm/lib/MC/MCAsmBackend.cpp
123

Probably should only return true if Sym->isExternal()?

oontvoo updated this revision to Diff 505889.Mar 16 2023, 11:36 AM
oontvoo marked an inline comment as done.

addressed review comment: check for isExternal() symbol

oontvoo edited the summary of this revision. (Show Details)Mar 16 2023, 11:39 AM
oontvoo updated this revision to Diff 507370.Mar 22 2023, 8:32 AM

Updated patch:

  • added LLD change + test
  • flip the new flag's default value to true to preserve current behaviour (easier to flip it to false and do test clean up separately)
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2023, 8:32 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
jyknight added inline comments.Mar 22 2023, 11:43 AM
lld/MachO/UnwindInfoSection.cpp
365 ↗(On Diff #507370)

I believe we still need to store functionLength, otherwise the computation of cueEndBoundary in later code will be wrong.

llvm/lib/MC/MCContext.cpp
934

IMO, we should flip this to start with -- I don't think there's a real point in doing it separately.

oontvoo updated this revision to Diff 507744.Mar 23 2023, 8:14 AM
oontvoo marked an inline comment as done.

Updated diff:

  • flip the emit-compact-unwind-non-canonical flag's default value back to false
  • have all the generateCompactUnwind...() return MODE_DWARF (instead of 0, that was a bug).

Tested this manually. Seemed to work!
The test was:

(0) Generate an asm file from:

#include <iostream>
#include <stdexcept>
#include "unwind.h"
 
typedef uint32_t _Unwind_State;
extern "C" _Unwind_Reason_Code __gxx_personality_v0(_Unwind_State state,
						    _Unwind_Exception* unwind_exception,
						    _Unwind_Context* context);
 
 _Unwind_Reason_Code
_my_personality(_Unwind_State state,
		_Unwind_Exception* unwind_exception,
		_Unwind_Context* context) {
   return __gxx_personality_v0(state, unwind_exception, context);
}
 
void func() {
  throw std::runtime_error("RUNTIME error");
}
 
int main() {
    try {
    	func();
    } catch(std::runtime_error ex){
        std::cout << "caught \n";
    }
    
    try {
      func();
    }
    catch (int i) { }
 
 
  return 0;
}

(1) Manually edit the asm to change main's personality function from __gxx_personality_v0 to _my_personality
(2) Assemble/link/execute it.
(3) Run the binary and got the following, which confirmed that DWARF unwind worked.

% ./manual.out 
caught 
libc++abi: terminating with uncaught exception of type std::runtime_error: RUNTIME error
zsh: abort      ./manual..out

(4) Extra verification: looking at the objdump to confirm that DWARF entries are emitted (rather than compact-unwind) for _main

oontvoo updated this revision to Diff 507749.Mar 23 2023, 8:18 AM
oontvoo marked an inline comment as done.

(updated diff - forgot to include LLD's requested LLD change)

int3 added inline comments.Mar 23 2023, 9:09 AM
lld/MachO/UnwindInfoSection.cpp
364 ↗(On Diff #507749)

seems like this was accidentally deleted?

366 ↗(On Diff #507749)

if we're not setting the personality, maybe the LSDA pointer isn't necessary either?

lld/test/MachO/compact-unwind-both-local-and-dylib-personality.s
1 ↗(On Diff #507370)

stray >

llvm/lib/MC/MCAsmBackend.cpp
122

wonder why the test didn't catch this

also couldn't this be a regular == string compare?

oontvoo updated this revision to Diff 517294.Apr 26 2023, 1:29 PM
oontvoo marked 2 inline comments as done.Apr 26 2023, 1:29 PM

new change:

  • updated tests to set the flag that emits compact-unwind for non-canonical to preserve old testings
  • added new tests that does NOT emit compact-unwind for non-canonical
oontvoo updated this revision to Diff 517681.Apr 27 2023, 12:30 PM

fix build warnings (struct vs class in forward decl)

oontvoo updated this revision to Diff 518710.May 2 2023, 7:06 AM

updated clang test

Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2023, 7:06 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
oontvoo updated this revision to Diff 519216.May 3 2023, 12:47 PM
  • defined equivalent flag in the clang driver

I wonder if we actually need to define a clang frontend flag for this; I suspect nobody will ever want to specify it, since the only non-canonical personality clang will ever generate that changes behavior is the pure-C destructor-only personality, __gnu_personality_v0. Otherwise, it could only arise from assembly or IR.

For a test-case, can just use -mllvm -emit-compact-unwind-non-canonical.

llvm/lib/MC/MCAsmBackend.cpp
119

This could use a comment as to why these two are "canonical" and why "__gcc_personality_v0" is excluded despite being similar.

oontvoo updated this revision to Diff 519849.May 5 2023, 7:26 AM

updated more clang test

oontvoo marked an inline comment as done.May 5 2023, 10:41 AM

I wonder if we actually need to define a clang frontend flag for this; I suspect nobody will ever want to specify it, since the only non-canonical personality clang will ever generate that changes behavior is the pure-C destructor-only personality, __gnu_personality_v0. Otherwise, it could only arise from assembly or IR.

For a test-case, can just use -mllvm -emit-compact-unwind-non-canonical.

Per offline discussion, passing it via -mllvm doesn't work because llvm-flags that populate MCTargetOptions flags aren't registered in Clang, so it needs to be at least a clang cc1 option.

lld/MachO/UnwindInfoSection.cpp
364 ↗(On Diff #507749)

yes - good catch

oontvoo updated this revision to Diff 519939.May 5 2023, 12:00 PM

addressed review comments:

  • added explanation on why we left out _gcc personality
  • updated more clang tests

hopefully should be all green now!

Hi all! The CI builds are all clean now. Any further comment/review on this?
Would love to close this out soon. Thanks! :)

oontvoo retitled this revision from [RFC][MC][MachO]Only emits compact-unwind format for "canonical" personality symbols. For the rest, use DWARFs. to [Clang][MC][MachO]Only emits compact-unwind format for "canonical" personality symbols. For the rest, use DWARFs..May 9 2023, 11:35 AM
oontvoo edited the summary of this revision. (Show Details)

@int3 @thakis, et al - friendly 🔔 :)

int3 accepted this revision.May 17 2023, 10:00 AM

lgtm

lld/MachO/UnwindInfoSection.cpp
346 ↗(On Diff #520344)

looks like the old comment can be retained

This revision is now accepted and ready to land.May 17 2023, 10:00 AM
oontvoo updated this revision to Diff 523147.May 17 2023, 1:12 PM
oontvoo marked an inline comment as done.
oontvoo edited the summary of this revision. (Show Details)

updated comment

oontvoo updated this revision to Diff 523354.May 18 2023, 6:12 AM

rebase again (windows failures in flang tests looked unrelated)

This makes lld crash when linking a libc++ with object files built by a clang that also contains the patch. Here's a reduced (to just libc++), stand-alone repro that repros the crash in 6 seconds on my m1 mbp: https://bugs.chromium.org/p/chromium/issues/detail?id=1446967#c10

Reverted in 4980eead4d0b4666d53dad07afb091375b3a13a0 for now.

(The assert only happens when targeting x86_64, not arm64. The repro script above has the right -arch flag to toggle it.)

oontvoo reopened this revision.May 31 2023, 11:28 AM

Reverted in 4980eead4d0b4666d53dad07afb091375b3a13a0 for now.

(The assert only happens when targeting x86_64, not arm64. The repro script above has the right -arch flag to toggle it.)

Thanks for the revert! This was caused by another bug (should be fixed in https://reviews.llvm.org/D151824)
Will reland this once that is landed.

llvm/lib/MC/MCAsmBackend.cpp
125

need an additional _

This revision is now accepted and ready to land.May 31 2023, 11:28 AM
oontvoo updated this revision to Diff 528965.Jun 6 2023, 11:44 AM

rebase for rolling forward. (Will wait ~1 day before pushing to avoid possible stacking reverts because this depends on D151824)

oontvoo closed this revision.Jun 7 2023, 7:07 AM

I believe this is breaking the LLDB tests:
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/56106/changes
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/buildTimeTrend

Build 56105 was passing, and the only relevant change seems to come from this review.

Note that the failure is due to a test that is now passing:

Unexpectedly Passed Tests (1):
  lldb-shell :: Unwind/prefer-debug-over-eh-frame.test

Maybe we forgot to re-enable that test?

I believe this is breaking the LLDB tests:
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/56106/changes
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/buildTimeTrend

Build 56105 was passing, and the only relevant change seems to come from this review.

Note that the failure is due to a test that is now passing:

Unexpectedly Passed Tests (1):
  lldb-shell :: Unwind/prefer-debug-over-eh-frame.test

Maybe we forgot to re-enable that test?

The behaviour has changed - I can update the test.
Question: Do you prefer to have it set a flag that goes back to the old behaviour? OR change to to expect-passing?

(My vote would be on have it expect the test to pass because, from reading the test, this is preferring debug-frame over eh-frame, which probably isn't ideal in the new code beacuse all non-canonical personality symbols will now be eh-frame)

I don't think this handles the no-personality case properly. For example this code leads to a DWARF entry now:

void bar(int *) noexcept;
void foo() {
  int arr;
  bar(&arr);
}
Michael137 added a subscriber: Michael137.EditedJun 9 2023, 4:12 AM

Looks like the latest reland of this patch (e60b30d5e3878e7d91f8872ec4c4dca00d4a2dfc) broke some debug-info cross-project-tests on the Arm64 macOS buildbots: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-as/263/execution/node/54/log/

Failed Tests (2):
  cross-project-tests :: debuginfo-tests/dexter-tests/optnone-fastmath.cpp
  cross-project-tests :: debuginfo-tests/dexter-tests/optnone-struct-and-methods.cpp

You can run those tests by adding cross-project-tests to LLVM_ENABLE_PROJECTS and running ninja check-debuginfo.

AFAICT, it also broke following LLDB test (https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-as/263/execution/node/65/log/):

Failed Tests (1):
  lldb-api :: functionalities/step-avoids-no-debug/TestStepNoDebug.py

Let me know if you need help reproducing this

I don't think this handles the no-personality case properly. For example this code leads to a DWARF entry now:

void bar(int *) noexcept;
void foo() {
  int arr;
  bar(&arr);
}

Thanks! Sent the fix in D152540

Looks like the latest reland of this patch (e60b30d5e3878e7d91f8872ec4c4dca00d4a2dfc) broke some debug-info cross-project-tests on the Arm64 macOS buildbots: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-as/263/execution/node/54/log/

Failed Tests (2):
  cross-project-tests :: debuginfo-tests/dexter-tests/optnone-fastmath.cpp
  cross-project-tests :: debuginfo-tests/dexter-tests/optnone-struct-and-methods.cpp

You can run those tests by adding cross-project-tests to LLVM_ENABLE_PROJECTS and running ninja check-debuginfo.

AFAICT, it also broke following LLDB test (https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-as/263/execution/node/65/log/):

Failed Tests (1):
  lldb-api :: functionalities/step-avoids-no-debug/TestStepNoDebug.py

Let me know if you need help reproducing this

Mind taking a look or reverting the patch until the tests pass?

Looks like the latest reland of this patch (e60b30d5e3878e7d91f8872ec4c4dca00d4a2dfc) broke some debug-info cross-project-tests on the Arm64 macOS buildbots: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-as/263/execution/node/54/log/

Failed Tests (2):
  cross-project-tests :: debuginfo-tests/dexter-tests/optnone-fastmath.cpp
  cross-project-tests :: debuginfo-tests/dexter-tests/optnone-struct-and-methods.cpp

You can run those tests by adding cross-project-tests to LLVM_ENABLE_PROJECTS and running ninja check-debuginfo.

AFAICT, it also broke following LLDB test (https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-as/263/execution/node/65/log/):

Failed Tests (1):
  lldb-api :: functionalities/step-avoids-no-debug/TestStepNoDebug.py

Let me know if you need help reproducing this

Mind taking a look or reverting the patch until the tests pass?

Yes - having a look now - wasn't able to repro the test failure before because the build broke at HEAD:

/mnt/ssd/repo/llvm-project/llvm/include/llvm/CodeGen/RDFRegisters.h: In member function ‘constexpr size_t llvm::rdf::RegisterRef::hash() const’:
/mnt/ssd/repo/llvm-project/llvm/include/llvm/CodeGen/RDFRegisters.h:92:35: error: call to non-‘constexpr’ function ‘std::size_t std::hash<unsigned int>::operator()(unsigned int) const’
   92 |     return std::hash<RegisterId>{}(Reg) ^
      |            ~~~~~~~~~~~~~~~~~~~~~~~^~~~~

P.S This might have the same root-cause with the previous comment and could be fixed by D152540 as well.
(not able to repro the failures yet - don't have arm64 readily available, will test it later this evening. but on linux x86-64 and macos x86-64, ninja check-debuginfo passed for me)

Michael137 added a comment.EditedJun 12 2023, 9:36 AM

P.S This might have the same root-cause with the previous comment and could be fixed by D152540 as well.
(not able to repro the failures yet - don't have arm64 readily available, will test it later this evening. but on linux x86-64 and macos x86-64, ninja check-debuginfo passed for me)

FYI, still failing with D152540. I'm able to repro this on my arm64 machine.

The repro without running dexter is as follows:

./bin/clang++ cross-project-tests/debuginfo-tests/dexter-tests/optnone-struct-and-methods.cpp -g -O0 -o test.out
./bin/lldb test.out -o "br se -l 90" -o run -o s -o s -o s -o s -o "expr _data.b.other_b"
(lldb) expr _data.b.other_b 
(A::B) $0 = 0x10d50

We would however expect the value to be B_VALUE

The backtrace also looks wonky:

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = step in
  * frame #0: 0x0000000100003d18 test.out`(anonymous namespace)::A::getData(this=0x0000000100011910) at optnone-struct-and-methods.cpp:87:5
    frame #1: 0x0000000187185058 dyld`start + 2224

Michael and I looked into this. This simple c++ file is resulting in eh_frame unwind info on aarch64 darwin instead of compact unwind info. The eh_frame instructions don't describe the epilogue, so lldb is stopping on the final RET instruction after it has adjusted $sp back to its original value and the unwind now fails to work in the debugger. That's always the problem with eh_frame/debug_frame from the debugger's point of view - it's only guaranteed to describe the unwind state at throwable locations. gdb lives exclusively off of eh_frame/debug_frame so in practice gcc/clang (at least on intel) describe both the prologue and the epilogue for the debugger's benefit. But that's not what this eh_frame includes on aarch64 darwin, and it breaks the debugger because it's trusting the eh_frame to be accurate at every instruction point.

The emission of eh_frame on aarch64 darwin instead of compact unwind, for this simple codegen, is a bug and cannot remain in the tree unfixed. It will increase binary size and reduce throw performance. It also happens to cause lldb regressions like this because we've never lived off of eh_frame as our primary unwind format on this platform, but that's a separate issue and I'm not going to dig in to finding a way to detect this & ignore the eh_frame on this target.

Looking at the failing test case that Michael has been debugging, none of these methods have a personality, and yet we're getting eh_frame instead of compact unwind. That's the bug.

Michael and I looked into this. This simple c++ file is resulting in eh_frame unwind info on aarch64 darwin instead of compact unwind info. The eh_frame instructions don't describe the epilogue, so lldb is stopping on the final RET instruction after it has adjusted $sp back to its original value and the unwind now fails to work in the debugger. That's always the problem with eh_frame/debug_frame from the debugger's point of view - it's only guaranteed to describe the unwind state at throwable locations. gdb lives exclusively off of eh_frame/debug_frame so in practice gcc/clang (at least on intel) describe both the prologue and the epilogue for the debugger's benefit. But that's not what this eh_frame includes on aarch64 darwin, and it breaks the debugger because it's trusting the eh_frame to be accurate at every instruction point.

The emission of eh_frame on aarch64 darwin instead of compact unwind, for this simple codegen, is a bug and cannot remain in the tree unfixed. It will increase binary size and reduce throw performance. It also happens to cause lldb regressions like this because we've never lived off of eh_frame as our primary unwind format on this platform, but that's a separate issue and I'm not going to dig in to finding a way to detect this & ignore the eh_frame on this target.

Thanks for the details! From your descriptions, I agree that this is a bug - the change in this patch is *NOT* intended to change the unwind format when there's no personality symbol - (it should only affect custom personality symbols, which is not the case in this example)
Still debugging - not sure why it only occurs for aarch64

Looking at the failing test case that Michael has been debugging, none of these methods have a personality, and yet we're getting eh_frame instead of compact unwind. That's the bug.

Michael and I looked into this. This simple c++ file is resulting in eh_frame unwind info on aarch64 darwin instead of compact unwind info.

P.S: still no luck reproducing this with TOT clang. Would you mind verifying my test case below (this was on M1 mac):

### clang is built with commit up to 5b1c62c0f2e9a739707429f650cb897c067f86c2
vyng-macbookpro3 /Users/vyng/repo/llvm-project/build_all$ ./bin/clang++  ../cross-project-tests/debuginfo-tests/dexter-tests/optnone-struct-and-methods.cpp -g -O0 -c -o test_tot.o

vyng-macbookpro3 /Users/vyng/repo/llvm-project/build_all$ objdump --macho --unwind-info --dwarf=frames  test_tot.o
Contents of __compact_unwind section:
  Entry at offset 0x0:
    start:                0x0 ltmp0
    length:               0x70
    compact encoding:     0x44000000
    personality function: 0x0 ___gxx_personality_v0
    LSDA:                 0x238 ltmp1
  Entry at offset 0x20:
    start:                0x70 __ZN12_GLOBAL__N_11AC1Ev
    length:               0x2c
    compact encoding:     0x04000000
  Entry at offset 0x40:
    start:                0x9c __ZN12_GLOBAL__N_11A7getDataEv
    length:               0x78
    compact encoding:     0x04000000
  Entry at offset 0x60:
    start:                0x114 __ZN12_GLOBAL__N_11AD1Ev
    length:               0x2c
    compact encoding:     0x04000000
  Entry at offset 0x80:
    start:                0x140 __ZN12_GLOBAL__N_11AC2Ev
    length:               0x2c
    compact encoding:     0x02001000
  Entry at offset 0xa0:
    start:                0x16c __ZN12_GLOBAL__N_11A12setOtherDataEv
    length:               0x74
    compact encoding:     0x04000000
  Entry at offset 0xc0:
    start:                0x1e0 __ZN12_GLOBAL__N_11A12getOtherDataEv
    length:               0x18
    compact encoding:     0x02001000
  Entry at offset 0xe0:
    start:                0x1f8 __ZN12_GLOBAL__N_11AD2Ev
    length:               0x40
    compact encoding:     0x04000000

.debug_frame contents:


.eh_frame contents:
<<<< nothing in eh_frame section, which is expected and matches what produced by base clang

Michael and I looked into this. This simple c++ file is resulting in eh_frame unwind info on aarch64 darwin instead of compact unwind info.

P.S: still no luck reproducing this with TOT clang. Would you mind verifying my test case below (this was on M1 mac):

### clang is built with commit up to 5b1c62c0f2e9a739707429f650cb897c067f86c2
vyng-macbookpro3 /Users/vyng/repo/llvm-project/build_all$ ./bin/clang++  ../cross-project-tests/debuginfo-tests/dexter-tests/optnone-struct-and-methods.cpp -g -O0 -c -o test_tot.o

vyng-macbookpro3 /Users/vyng/repo/llvm-project/build_all$ objdump --macho --unwind-info --dwarf=frames  test_tot.o
Contents of __compact_unwind section:
  Entry at offset 0x0:
    start:                0x0 ltmp0
    length:               0x70
    compact encoding:     0x44000000
    personality function: 0x0 ___gxx_personality_v0
    LSDA:                 0x238 ltmp1
  Entry at offset 0x20:
    start:                0x70 __ZN12_GLOBAL__N_11AC1Ev
    length:               0x2c
    compact encoding:     0x04000000
  Entry at offset 0x40:
    start:                0x9c __ZN12_GLOBAL__N_11A7getDataEv
    length:               0x78
    compact encoding:     0x04000000
  Entry at offset 0x60:
    start:                0x114 __ZN12_GLOBAL__N_11AD1Ev
    length:               0x2c
    compact encoding:     0x04000000
  Entry at offset 0x80:
    start:                0x140 __ZN12_GLOBAL__N_11AC2Ev
    length:               0x2c
    compact encoding:     0x02001000
  Entry at offset 0xa0:
    start:                0x16c __ZN12_GLOBAL__N_11A12setOtherDataEv
    length:               0x74
    compact encoding:     0x04000000
  Entry at offset 0xc0:
    start:                0x1e0 __ZN12_GLOBAL__N_11A12getOtherDataEv
    length:               0x18
    compact encoding:     0x02001000
  Entry at offset 0xe0:
    start:                0x1f8 __ZN12_GLOBAL__N_11AD2Ev
    length:               0x40
    compact encoding:     0x04000000

.debug_frame contents:


.eh_frame contents:
<<<< nothing in eh_frame section, which is expected and matches what produced by base clang

Sorry for the back-and-forth
Looks like D152540 did indeed resolve this. The bots apparently didn't pick that change up until today. And locally I must've been producing the binary without that commit

It's all good now, thanks!