[PATCH] D16009: [lld]Non-MachO references shouldn't assert when checking for TLV.

[Lang Hames via llvm-commits]

lhames added inline comments.

================
Comment at: lib/ReaderWriter/MachO/ArchHandler_x86_64.cpp:63
@@ -62,2 +62,3 @@
   bool isTLVAccess(const Reference &ref) const override {
-    assert(ref.kindNamespace() == Reference::KindNamespace::mach_o);
+    if (ref.kindNamespace() != Reference::KindNamespace::mach_o)
+      return false;
----------------
grosbach wrote:
> lhames wrote:
> > echristo wrote:
> > > Can you add a comment explaining how you could get here? That said, is the check worth it now? i.e. do we really need the early exit?
> > This is consistent with how GOT references (which are handled very similarly) are checked. We may try to improve on it in the future for performance reasons, but I'm happy with it for now.
> A comment to that effect in the code seems a totally reasonable thing. The very fact that this patch is necessary is evidence that it's not completely obvious.
There are two things going on here:

1. When you check the kindValue you need to interpret it in the context of the kindNamespace and kindArch. We should have something to encapsulate that check so that it can't be accidentally dropped (causing another bug like this). There may already be something like this, I haven't gone looking for it yet.

2. I think ripRel32Tlv is generic, but currently its namespace and arch are getting set to mach_o x86_64 . They should probably be set to all/all instead (though I'll have to sanity check that), in which case we wouldn't need to query the ArchHandler about this at all.

Given how common this kind of check is, commenting this here probably isn't the right option. I'll file bugs to look into (1) and (2) instead.


Repository:
  rL LLVM

http://reviews.llvm.org/D16009