[PATCH] D151824: [lld-macho]Fixed bug folding LSDA incorrectly

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo marked an inline comment as done and an inline comment as not done.
oontvoo added a comment.

In D151824#4393486 <https://reviews.llvm.org/D151824#4393486>, @int3 wrote:

> +1 to what @smeenai said

I've updated the tests to add more assertions. But originally, the fact that the test didn't crash was sort of enough to cover to bug this aimed to fix.

> Also re your commit message:
>
>> With assertion turned-off, lld would produce "bad" binary in which the gcc_except_table* got folded but not the related functions:
>
> Is this actually a problem? Does the unwinder not like it when otherwise identical functions at two separate addresses share the same gcc_except_table?

Actually, the most serious issue here was that the linker crashed on the assertion  (stacktrace attached in the bug report).
The rest of the symptoms are probably not as alarming. (the description/commit message was jsut trying to point out that the entries were pointing at the bad address).
Anyway, I've updated the tests with more assertions - hopefully that would make it a bit clearer.

Thanks!



================
Comment at: lld/MachO/Writer.cpp:680-681
+      // Canonicalize the referent so that later accesses in Writer won't
+      // have to worry about it. Perhaps we should do this for Defined::isec
+      // too...
+      if (auto *referentIsec = r.referent.dyn_cast<InputSection *>())
----------------
int3 wrote:
> this is now being done in `scanSymbols` (I'm aware this was from an existing comment)
updated comment


================
Comment at: lld/MachO/Writer.cpp:693
         // to emit rebase opcodes for it.
-        it++;
+        skipNext = true;
         continue;
----------------
int3 wrote:
> I think just copypasting the call to `canonicalize()` here would be cleaner -- no need for the `skipNext` state + would take fewer lines of code too
couldn't simply copy the canonicalize() call here because we want to  call it on the "next" entry (ie., the "skipped" one) as well.

In other words this `it++` would have skipped over it .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151824