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

[oontvoo]

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).

[oontvoo]

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 diff - forgot to include LLD's requested LLD change)

[int3]

lld/MachO/UnwindInfoSection.cpp

361

seems like this was accidentally deleted?

367

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

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]

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]

rebase

[oontvoo]

fix build warnings (struct vs class in forward decl)

[oontvoo]

updated clang test

[oontvoo]

• defined equivalent flag in the clang driver

[jyknight]

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 more clang test

[oontvoo]

In D144999#4319964, @jyknight wrote:

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

361

yes - good catch

[oontvoo]

addressed review comments:

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

hopefully should be all green now!

[oontvoo]

rebase

[oontvoo]

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

[oontvoo]

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

[int3]

lgtm

lld/MachO/UnwindInfoSection.cpp

346–347

looks like the old comment can be retained

[oontvoo]

updated comment

[oontvoo]

rebase

[oontvoo]

Thanks!

[oontvoo]

rebase again (windows failures in flang tests looked unrelated)

[thakis]

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

[thakis]

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]

In D144999#4356218, @thakis wrote:

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 _

[oontvoo]

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

[oontvoo]

rebase

[fdeazeve]

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?

[oontvoo]

In D144999#4405793, @fdeazeve wrote:

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)

[oontvoo]

P.S: https://reviews.llvm.org/D152449

[t.p.northover]

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]

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

[oontvoo]

In D144999#4407934, @t.p.northover wrote:

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

[Michael137]

In D144999#4408248, @Michael137 wrote:

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?

[oontvoo]

In D144999#4409149, @Michael137 wrote:

In D144999#4408248, @Michael137 wrote:

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) ^
      |            ~~~~~~~~~~~~~~~~~~~~~~~^~~~~

[oontvoo]

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)