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

[mstorsjo]

Summary

The thumb bit should only be set for executable code.

[compnerd]

COFF/DLL.cpp

365

Please make this const.

369

Can you hoist this to the beginning of the loop? Basically,

if (E.Data)
  continue;

[mstorsjo]

COFF/DLL.cpp

365

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.

369

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

[compnerd]

COFF/DLL.cpp

365

Right, I realize that you are just moving it, but you are moving it, so seems like a good time to just fix that.

369

Oh, yeah, missed that due to the weird side-by-side diff.

COFF/DLL.cpp

 };
 
 class AddressTableChunk : public Chunk {
 public:
   explicit AddressTableChunk(size_t MaxOrdinal) : Size(MaxOrdinal + 1) {}
   size_t getSize() const override { return Size * 4; }
 
   void writeTo(uint8_t *Buf) const override {
+    for (const Export &E : Config->Exports) {
+      uint8_t *P = Buf + OutputSectionOff + E.Ordinal * 4;
       uint32_t Bit = 0;
       // Pointer to thumb code must have the LSB set, so adjust it.
-    if (Config->Machine == ARMNT)
+      if (Config->Machine == ARMNT && !E.Data)
         Bit = 1;
-    for (Export &E : Config->Exports) {
-      uint8_t *P = Buf + OutputSectionOff + E.Ordinal * 4;
       if (E.ForwardChunk) {
         write32le(P, E.ForwardChunk->getRVA() | Bit);
       } else {
         write32le(P, cast<Defined>(E.Sym)->getRVA() | Bit);
       }
     }
   }
 

test/COFF/export-armnt.yaml

 # REQUIRES: arm
 
 # RUN: yaml2obj < %s > %t.obj
 #
-# RUN: lld-link /out:%t.dll /dll %t.obj /export:exportfn1 /export:exportfn2
+# RUN: lld-link /out:%t.dll /dll %t.obj /export:exportfn1 /export:exportfn2 /export:exportdata,data
 # RUN: llvm-objdump -p %t.dll | FileCheck %s
 
 # CHECK:      Export Table:
 # CHECK:      DLL name: export-armnt.yaml.tmp.dll
 # CHECK:      Ordinal      RVA  Name
 # CHECK-NEXT:       0        0
-# CHECK-NEXT:       1   0x1005  exportfn1
-# CHECK-NEXT:       2   0x1009  exportfn2
-# CHECK-NEXT:       3   0x1009  exportfn3
+# CHECK-NEXT:       1   0x1000  exportdata
+# CHECK-NEXT:       2   0x2005  exportfn1
+# CHECK-NEXT:       3   0x2009  exportfn2
+# CHECK-NEXT:       4   0x2009  exportfn3
 
 --- !COFF
 header:
   Machine:         IMAGE_FILE_MACHINE_ARMNT
   Characteristics: []
 sections:
   - Name:            .text
     Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
     Alignment:       4
     SectionData:     704700bf704700bf704700bf
+  - Name:            .data
+    Characteristics: [ IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       4
+    SectionData:     00000000
   - Name:            .drectve
     Characteristics: [ IMAGE_SCN_LNK_INFO, IMAGE_SCN_LNK_REMOVE ]
     Alignment:       1
     SectionData:     2f6578706f72743a6578706f7274666e3300  # /export:exportfn3
 symbols:
   - Name:            .text
     Value:           0
     SectionNumber:   1
     SimpleType:      IMAGE_SYM_TYPE_NULL
     ComplexType:     IMAGE_SYM_DTYPE_NULL
     StorageClass:    IMAGE_SYM_CLASS_STATIC
     SectionDefinition:
       Length:          12
       NumberOfRelocations: 0
       NumberOfLinenumbers: 0
       CheckSum:        0
       Number:          0
+  - Name:            .data
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          4
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        0
+      Number:          0
   - Name:            _DllMainCRTStartup
     Value:           0
     SectionNumber:   1
     SimpleType:      IMAGE_SYM_TYPE_NULL
     ComplexType:     IMAGE_SYM_DTYPE_NULL
     StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
   - Name:            exportfn1
     Value:           4
     SectionNumber:   1
     SimpleType:      IMAGE_SYM_TYPE_NULL
     ComplexType:     IMAGE_SYM_DTYPE_NULL
     StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
   - Name:            exportfn2
     Value:           8
     SectionNumber:   1
     SimpleType:      IMAGE_SYM_TYPE_NULL
     ComplexType:     IMAGE_SYM_DTYPE_NULL
     StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
   - Name:            exportfn3
     Value:           8
     SectionNumber:   1
     SimpleType:      IMAGE_SYM_TYPE_NULL
     ComplexType:     IMAGE_SYM_DTYPE_NULL
     StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            exportdata
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
   - Name:            '?mangled@@YAHXZ'
     Value:           8
     SectionNumber:   1
     SimpleType:      IMAGE_SYM_TYPE_NULL
     ComplexType:     IMAGE_SYM_DTYPE_NULL
     StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
 ...