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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added inline comments.


================
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())
----------------
Why is `dyn_cast` used in 691 but `dyn_cast_or_null` here? (seems like the same use case to me )


================
Comment at: lld/test/MachO/compact-unwind-both-local-and-dylib-personality.s:42
 # BC-NEXT: address                    index name
-# BC-DAG:  0x[[#%x,GXX_PERSONALITY_HI:]] LOCAL
-# BC-DAG:  0x[[#%x,GXX_PERSONALITY_LO:]] LOCAL
-# BC-DAG:  0x[[#%x,PERSONALITY_1:]]  LOCAL
-# BC-DAG:  0x[[#%x,PERSONALITY_2:]]  LOCAL
+# C:       0x[[#%x,GXX_PERSONALITY_HI:]] LOCAL
+# BC:      0x[[#%x,GXX_PERSONALITY_LO:]] LOCAL
----------------
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)



================
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:
> 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109944