[PATCH] D130208: [lld-macho] Fix assertion when two symbols at same addr have unwind info

[Jez Ng via Phabricator via llvm-commits]

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


================
Comment at: lld/MachO/InputFiles.cpp:1536
+      // infrequently (only when handling the output of `ld -r`).
+      funcSym = findSymbolAtOffset(cast<ConcatInputSection>(funcSym->isec),
+                                   funcSym->value);
----------------
abrachet wrote:
> abrachet wrote:
> > int3 wrote:
> > > int3 wrote:
> > > > thakis wrote:
> > > > > We hit this assert in "normal" builds: https://bugs.chromium.org/p/chromium/issues/detail?id=1346125#c1
> > > > > 
> > > > > Do you want a repro file for that?
> > > > > 
> > > > > Anyways, addressing the assert quickly would be good :) (either revert if hitting this on a normal build is surprising and needs more investigation, or by landing this fix here)
> > > > Just to confirm, are you sure the "normal" build doesn't pull in any 3rd-party lib that could've been passed through `ld -r`?
> > > > 
> > > > I wasn't able to generate it the problem via llvm-mc in my testing, but I could certainly have missed something. Repro file wouldn't really help to figure out the compiler flags that trigger it though...
> > > > 
> > > > It doesn't look like the assert triggers at all on `chromium_framework` at least, so I think it's safe to say that it's a fairly uncommon code path
> > > > It doesn't look like the assert triggers at all on chromium_framework at least, so I think it's safe to say that it's a fairly uncommon code path
> > > 
> > > More precisely, an `assert(false)` in this `if` branch doesn't get tripped while linking chromium_framework.
> > > 
> > > Landing this now, we can tweak the comment as needed later
> > Hi this is crashing for us too I've uploaded a reproducer here https://drive.google.com/file/d/1tz3feLMfR-2N8SMxT5dqLqAN3dUVSGRQ/view?usp=sharing. Though it seems like not all input files correctly made it into the reproducer.
> > See https://ci.chromium.org/ui/p/fuchsia/builders/prod/clang-mac-xarm64/b8807891867843444577/overview which is an lto build of llvm. If it's helpful here's what `funcSym` looks like before the crash
> > ```
> > (lldb) p *funcSym
> > (lld::macho::Defined) $9 = {
> >   lld::macho::Symbol = {
> >     gotIndex = 4294967295
> >     lazyBindOffset = 4294967295
> >     stubsHelperIndex = 4294967295
> >     stubsIndex = 4294967295
> >     symtabIndex = 4294967295
> >     symbolKind = DefinedKind
> >     nameData = 0x000000011986014f "__ZNSt3__212basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEC1B6v15000IDnEEPKc"
> >     file = 0x000000011a849a00
> >     nameSize = 82
> >     isUsedInRegularObj = true
> >     used = false
> >   }
> >   overridesWeakDef = false
> >   privateExtern = true
> >   includeInSymtab = true
> >   wasIdenticalCodeFolded = false
> >   thumb = false
> >   referencedDynamically = false
> >   noDeadStrip = false
> >   interposable = false
> >   weakDefCanBeHidden = false
> >   weakDef = true
> >   external = true
> >   isec = nullptr
> >   value = 0
> >   size = 0
> >   unwindEntry = nullptr
> > }
> > ```
> Ignore the "too" in my comment. I incorrectly read @thakis's comment as him saying this was causing an assertion failure not fixing one.
whoops, guess it's bug whackamole season. Sorry about that. D130409 should fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130208