[PATCH] D41379: [LLD] [COFF] Don't set the thumb bit in address table entries for data symbols

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.


================
Comment at: COFF/DLL.cpp:365
   void writeTo(uint8_t *Buf) const override {
-    uint32_t Bit = 0;
-    // Pointer to thumb code must have the LSB set, so adjust it.
-    if (Config->Machine == ARMNT)
-      Bit = 1;
     for (Export &E : Config->Exports) {
       uint8_t *P = Buf + OutputSectionOff + E.Ordinal * 4;
----------------
mstorsjo wrote:
> compnerd wrote:
> > Please make this `const`.
> Well I'm not actually touching this line (contrary to what the phabricator diff shows), it's the Bit lines that I'm moving into the loop, so this is unrelated. A git/svn diff that does care about whitespace shows it differently.
Right, I realize that you are just moving it, but you are moving it, so seems like a good time to just fix that.


================
Comment at: COFF/DLL.cpp:369
+      // Pointer to thumb code must have the LSB set, so adjust it.
+      if (Config->Machine == ARMNT && !E.Data)
+        Bit = 1;
----------------
mstorsjo wrote:
> compnerd wrote:
> > Can you hoist this to the beginning of the loop?  Basically,
> > 
> >     if (E.Data)
> >       continue;
> No, we're not only setting the thumb bit here, we're also writing the full address in itself in this loop - in that case we wouldn't write any address at all for data symbols
Oh, yeah, missed that due to the weird side-by-side diff.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D41379