[PATCH] D109944: [lld-macho] Associate compact unwind entries with function symbols

[Jez Ng via Phabricator via llvm-commits]

int3 marked 2 inline comments as done.
int3 added inline comments.


================
Comment at: lld/MachO/InputSection.cpp:119
+    if ((*it)->value == v)
+      (*it)->compactUnwind = nullptr;
+    else
----------------
oontvoo wrote:
> do you want to assert the two `compactUnwind`'s are actually the same first?
in general the `compactUnwind`s won't have pointer equality, and doing a recursive comparison here seems like too much work


================
Comment at: lld/MachO/Writer.cpp:693
+      for (Symbol *sym : objFile->symbols) {
+        if (auto *defined = dyn_cast_or_null<Defined>(sym))
+          if (!defined->isExternal() && defined->isLive())
----------------
oontvoo wrote:
> Why is `dyn_cast` used in 691 but `dyn_cast_or_null` here? (seems like the same use case to me )
because `sym` can be null here. See ObjFile::parseSymbols -- not even index in the `symbols` array gets initialized.


================
Comment at: lld/test/MachO/compact-unwind-both-local-and-dylib-personality.s:42-45
+# C:       0x[[#%x,GXX_PERSONALITY_HI:]] LOCAL
+# BC:      0x[[#%x,GXX_PERSONALITY_LO:]] LOCAL
+# BC:      0x[[#%x,PERSONALITY_1:]]      LOCAL
+# BC:      0x[[#%x,PERSONALITY_2:]]      LOCAL
----------------
oontvoo wrote:
> oontvoo wrote:
> > int3 wrote:
> > > int3 wrote:
> > > > I removed the `-DAG` part here because it seems we are actually relying on the local symbols matching in exactly that order
> > > It seems that in the case of `b.out`, we don't emit this extra local binding. I haven't dug into why but I think it should be safe since that binding wasn't being used anyway
> > This is probably fine. 
> > (See also https://reviews.llvm.org/D111852#inline-1066410)
> > 
> Any guarantee that the LOCALs will get printed after non-locals?
> 
> While it's true that we rely on the partial order (ie., amongst the `LOCAL`s and amongst the `__gxx_personaliy`s ) but I wasn't sure we could rely on the ordering overall being deterministic
No guarantee per se -- i.e. it *could* change -- that's just how it's currently implemented. But yes the behavior should be deterministic (and if not it's a bug).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109944/new/

https://reviews.llvm.org/D109944