Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes

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
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 updated this revision to Diff 517294.Apr 26 2023, 1:29 PM
oontvoo marked 2 inline comments as done.

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
361

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.Wed, May 17, 10:00 AM

lgtm

lld/MachO/UnwindInfoSection.cpp
346–347

looks like the old comment can be retained

This revision is now accepted and ready to land.Wed, May 17, 10:00 AM
oontvoo updated this revision to Diff 523147.Wed, May 17, 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.Thu, May 18, 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.Wed, May 31, 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.Wed, May 31, 11:28 AM
oontvoo updated this revision to Diff 528965.Tue, Jun 6, 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.Wed, Jun 7, 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.EditedFri, Jun 9, 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)