[llvm-objdump][COFF] Skip empty entries when dumping the export table

[aganea]

Summary

Ditto. dumpbin /exports and binutils objdump do skip empty entries as well. This patch is to simplify an upcoming change in LLD.

[mstorsjo]

lld/test/COFF/export32.test

89

This change doesn't seem consistent and/or directly explainable by the change itself?

llvm/tools/llvm-objdump/COFFDump.cpp

570

This part of the change also seems a bit unrelated to the main change. If you want it bundled here, make sure to at least mention/explain it in the commit message.

[hans]

Can you share what future change this will simplify? The current output seems natural in a way.

[mstorsjo]

In D149610#4311751, @hans wrote:

Can you share what future change this will simplify? The current output seems natural in a way.

I think it might be D149611 - which is marked as a child revision, but doesn't seem to be made including the changes from this patch.

Currently, for these cases we do have empty stub entries in the export tables, but with D149611 we'll also be creating export tables that start from a different ordinal. I think it might be good to visually see the difference between those two cases?

[alvinhochun]

I don't think we should skip empty exports, especially not by checking only the RVA, because it looks like it is theoretically possible to have RVA = 0 but a non-empty name for an export. Even entries with null RVA and null name seems worth keeping as they do take up space in the export table.

I don't think we need to follow exactly the behaviour of dumpbin. Can you check what Binutils objdump does just for comparison?

[aganea]

@hans @alvinhochun The rationale is to simply remove noise from the output when there are sparse ordinals in the table. I'm not quite sure how useful is to keeps those empty entries, since they will be skipped over by the kernel anyway. The check in COFFObjectFile.cpp, L1567 checks if the RVA is in the output sections. If it's outside the sections, that entry isn't exporting anything, and there will be no dynamic linking at runtime.

In D149610#4311847, @alvinhochun wrote:

I don't think we need to follow exactly the behaviour of dumpbin. Can you check what Binutils objdump does just for comparison?

objdump skips over empty values, as this patch does (using a test added by D149611):

C:\git\llvm-project>objdump --version
GNU objdump (Binutils for MinGW-W64 x86_64, built by Brecht Sanders) 2.36.1
Copyright (C) 2021 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or (at your option) any later version.
This program has absolutely no warranty.

C:\git\llvm-project>objdump -p "C:\git\llvm-project\stage1_msvc_debug\tools\lld\test\COFF\Output\ordinals-override.test.tmp.dll"
...
The Export Tables (interpreted .rdata section contents)

Export Flags                    0
Time/Date stamp                 0
Major/Minor                     0/0
Name                            0000000000016848 ordinals-override.test.tmp.dll
Ordinal Base                    122
Number in:
        Export Address Table            00002698
        [Name Pointer/Ordinal] Table    00000003
Table Addresses
        Export Address Table            0000000000016867
        Name Pointer Table              00000000000202c7
        Ordinal Table                   00000000000202d3

Export Address Table -- Ordinal Base 122
        [   0] +base[ 122] 1010 Export RVA
        [9878] +base[10000] 1000 Export RVA
        [9879] +base[10001] 1020 Export RVA

[Ordinal/Name Pointer] Table
        [   0] ?bar@@YAXXZ
        [9878] ?foo@@YAXXZ
        [9879] baz

lld/test/COFF/export32.test

89

The previous pattern was wrong. The RVA 0 was actually matching the first character of 0x1010.

llvm/tools/llvm-objdump/COFFDump.cpp

570

I was planning to commit it separately as a NFC patch, is that ok with you?

[mstorsjo]

lld/test/COFF/export32.test

89

Ok, that makes sense. Please mention and explain that bit in the commit message then!

llvm/tools/llvm-objdump/COFFDump.cpp

570

Yes, that sounds good, I’d rather have that as a separate commit.

[aganea]

lld/test/COFF/export32.test

89

I could separate that in a commit as well, since it's not relevant to this patch.

[jhenderson]

llvm/lib/Object/COFFObjectFile.cpp

1568–1570 ↗

(On Diff #518530)

Drive-by comments as I noticed these when my herald rule triggered:

I believe static_cast is generally preferred to C-style casts.

Why consumeError rather than not return E? I guess it's because the interesting bit is whether or not getRvaPtr fails, but is there a risk it will fail for some other reason (I haven't looked at the underlying code at all, so there might not be)? Also, would it be better for this method to return Expected<bool> rather than pass in an input parameter for the result?

[hans]

In D149610#4312974, @aganea wrote:

@hans @alvinhochun The rationale is to simply remove noise from the output when there are sparse ordinals in the table.

Fair enough. If sparse tables are a thing and other tools do it too, I suppose that makes sense.

[mstorsjo]

In D149610#4314907, @hans wrote:

In D149610#4312974, @aganea wrote:

@hans @alvinhochun The rationale is to simply remove noise from the output when there are sparse ordinals in the table.

Fair enough. If sparse tables are a thing and other tools do it too, I suppose that makes sense.

As long as the tool does print what the actual initial value was (which iirc it does?) I guess it makes sense.

[alvinhochun]

In D149610#4312974, @aganea wrote:

@hans @alvinhochun The rationale is to simply remove noise from the output when there are sparse ordinals in the table. I'm not quite sure how useful is to keeps those empty entries, since they will be skipped over by the kernel anyway. [...]

In D149610#4311847, @alvinhochun wrote:

I don't think we need to follow exactly the behaviour of dumpbin. Can you check what Binutils objdump does just for comparison?

objdump skips over empty values, as this patch does (using a test added by D149611):

Fair enough. Hiding completely empty entries may not be so bad then, given that two other tools do the same

The check in COFFObjectFile.cpp, L1567 checks if the RVA is in the output sections. If it's outside the sections, that entry isn't exporting anything, and there will be no dynamic linking at runtime.

Still, I don't quite agree with hiding any entries with non-zero RVA or non-empty names, even if they may not be linked at runtime. My opinion is that if an export entry has either, then it is likely they have been deliberately there for a reason or possibly created in error, so they should still be shown in the output for reference. (I think you can get such symbols using -export:zeroRvaExport=__ImageBase.)

So, I think the condition for skipping should only be RVA == 0 && Name.empty(). I would also consider doing the checks directly in the for loop without adding the ExportDirectoryEntryRef::isEmpty function. Just move move I->getSymbolNameearlier so you can check the RVA and the name before printing the entry.

lld/test/COFF/export.test

16–20

I'm thinking, since you are removing the empty entries in this patch, perhaps it would be a good idea to add checks for Ordinal base: 1 here also? This way the change to the ordinal base in the follow-up patch will be clearer.

[aganea]

As suggested.

[aganea]

In D149610#4315001, @alvinhochun wrote:

In D149610#4312974, @aganea wrote:

@hans @alvinhochun The rationale is to simply remove noise from the output when there are sparse ordinals in the table. I'm not quite sure how useful is to keeps those empty entries, since they will be skipped over by the kernel anyway. [...]

In D149610#4311847, @alvinhochun wrote:

I don't think we need to follow exactly the behaviour of dumpbin. Can you check what Binutils objdump does just for comparison?

objdump skips over empty values, as this patch does (using a test added by D149611):

Fair enough. Hiding completely empty entries may not be so bad then, given that two other tools do the same

The check in COFFObjectFile.cpp, L1567 checks if the RVA is in the output sections. If it's outside the sections, that entry isn't exporting anything, and there will be no dynamic linking at runtime.

Still, I don't quite agree with hiding any entries with non-zero RVA or non-empty names, even if they may not be linked at runtime. My opinion is that if an export entry has either, then it is likely they have been deliberately there for a reason or possibly created in error, so they should still be shown in the output for reference. (I think you can get such symbols using -export:zeroRvaExport=__ImageBase.)

So, I think the condition for skipping should only be RVA == 0 && Name.empty(). I would also consider doing the checks directly in the for loop without adding the ExportDirectoryEntryRef::isEmpty function. Just move move I->getSymbolNameearlier so you can check the RVA and the name before printing the entry.

I changed as you said. Although binutils had just RVA == 0 for the past 24 years at least (I couldn't trace back to the source of the initial commit for that line): https://github.com/gittup/binutils/blame/gittup/bfd/peXXigen.c#L1524

lld/test/COFF/export.test

16–20

I did it in the other patch, but I could add it here earlier.

llvm/lib/Object/COFFObjectFile.cpp

1568–1570 ↗

(On Diff #518530)

I ended up removing this code, see other comment.

[alvinhochun]

Thanks, LGTM.

lld/test/COFF/export.test

 # RUN: yaml2obj %p/Inputs/export.yaml -o %t.obj
 #
 # RUN: lld-link /out:%t.dll /dll %t.obj /export:exportfn1 /export:exportfn2
 # RUN: llvm-objdump -p %t.dll | FileCheck --check-prefix=CHECK1 %s
 
 CHECK1:      Export Table:
-CHECK1:      DLL name: export.test.tmp.dll
-CHECK1:      Ordinal      RVA  Name
+CHECK1-NEXT: DLL name: export.test.tmp.dll
+CHECK1-NEXT: Ordinal base: 1
+CHECK1-NEXT: Ordinal      RVA  Name
 CHECK1-NEXT:       1   0x1008  exportfn1
 CHECK1-NEXT:       2   0x1010  exportfn2
 
 # RUN: lld-link /out:%t.dll /dll %t.obj /export:exportfn1,@5 /export:exportfn2
 # RUN: llvm-objdump -p %t.dll | FileCheck --check-prefix=CHECK2 %s
 
 CHECK2:      Export Table:
-CHECK2:      DLL name: export.test.tmp.dll
-CHECK2:      Ordinal      RVA  Name
-CHECK2-NEXT:       1        0
-CHECK2-NEXT:       2        0
-CHECK2-NEXT:       3        0
-CHECK2-NEXT:       4        0
+CHECK2-NEXT: DLL name: export.test.tmp.dll
+CHECK2-NEXT: Ordinal base: 1
+CHECK2-NEXT: Ordinal      RVA  Name
 CHECK2-NEXT:       5   0x1008  exportfn1
 CHECK2-NEXT:       6   0x1010  exportfn2
 CHECK2-NEXT:       7   0x1010  exportfn3
 
 # RUN: lld-link /out:%t.dll /dll %t.obj /export:exportfn1,@5,noname /export:exportfn2
 # RUN: llvm-objdump -p %t.dll | FileCheck --check-prefix=CHECK3 %s
 
 CHECK3:      Export Table:
-CHECK3:      DLL name: export.test.tmp.dll
-CHECK3:      Ordinal      RVA  Name
-CHECK3-NEXT:       1        0
-CHECK3-NEXT:       2        0
-CHECK3-NEXT:       3        0
-CHECK3-NEXT:       4        0
+CHECK3-NEXT: DLL name: export.test.tmp.dll
+CHECK3-NEXT: Ordinal base: 1
+CHECK3-NEXT: Ordinal      RVA  Name
 CHECK3-NEXT:       5   0x1008
 CHECK3-NEXT:       6   0x1010  exportfn2
 
 # RUN: lld-link /out:%t.dll /dll %t.obj /export:f1=exportfn1 /export:f2=exportfn2 /implib:%t.lib
 # RUN: llvm-objdump -p %t.dll | FileCheck --check-prefix=CHECK4 %s
 # RUN: llvm-nm %t.lib | FileCheck -check-prefix=CHECK4-NM %s
 
 CHECK4:      Export Table:
-CHECK4:      DLL name: export.test.tmp.dll
-CHECK4:      Ordinal      RVA  Name
+CHECK4-NEXT: DLL name: export.test.tmp.dll
+CHECK4-NEXT: Ordinal base: 1
+CHECK4-NEXT: Ordinal      RVA  Name
 CHECK4-NEXT:       1   0x1010  exportfn3
 CHECK4-NEXT:       2   0x1008  f1
 CHECK4-NEXT:       3   0x1010  f2
 CHECK4-NM: 00000000 T f1
 CHECK4-NM: 00000000 T f2
 
 # RUN: echo "EXPORTS exportfn1 @3" > %t.def
 # RUN: echo "fn2=exportfn2 @2" >> %t.def
 # RUN: lld-link /out:%t.dll /dll %t.obj /def:%t.def
 # RUN: llvm-objdump -p %t.dll | FileCheck --check-prefix=CHECK5 %s
 
 CHECK5:      Export Table:
-CHECK5:      DLL name: export.test.tmp.dll
-CHECK5:      Ordinal      RVA  Name
-CHECK5-NEXT:       1        0
+CHECK5-NEXT: DLL name: export.test.tmp.dll
+CHECK5-NEXT: Ordinal base: 1
+CHECK5-NEXT: Ordinal      RVA  Name
 CHECK5-NEXT:       2   0x1010  fn2
 CHECK5-NEXT:       3   0x1008  exportfn1
 CHECK5-NEXT:       4   0x1010  exportfn3
 
 # RUN: lld-link /out:%t.DLL /dll %t.obj /export:exportfn1 /export:exportfn2 \
 # RUN:   /export:exportfn1 /export:exportfn2,@5 >& %t.log
 # RUN: FileCheck -check-prefix=CHECK6 %s < %t.log
 

lld/test/COFF/export32.test

 # RUN: yaml2obj %s -o %t.obj
 #
 # RUN: lld-link -safeseh:no /out:%t.dll /dll %t.obj /export:exportfn1 /export:exportfn2
 # RUN: llvm-objdump -p %t.dll | FileCheck --check-prefix=CHECK1 %s
 #
 # RUN: lld-link -safeseh:no /out:%t.dll /dll %t.obj /export:exportfn1 /export:exportfn2 /merge:.edata=.rdata
 # RUN: llvm-objdump -p %t.dll | FileCheck --check-prefix=CHECK1 %s
 # RUN: llvm-readobj --file-headers --sections %t.dll | FileCheck -check-prefix=HEADER-MERGE %s
 
 # CHECK1:      Export Table:
-# CHECK1:      DLL name: export32.test.tmp.dll
-# CHECK1:      Ordinal      RVA  Name
+# CHECK1-NEXT: DLL name: export32.test.tmp.dll
+# CHECK1-NEXT: Ordinal base: 1
+# CHECK1-NEXT: Ordinal      RVA  Name
 # CHECK1-NEXT:       1   0x1008  exportfn1
 # CHECK1-NEXT:       2   0x1010  exportfn2
 
 # HEADER-MERGE: ExportTableRVA: 0x2000
 # HEADER-MERGE-NEXT: ExportTableSize: 0x7A
 # HEADER-MERGE: Name: .rdata
 # HEADER-MERGE-NEXT: VirtualSize: 0x7A
 # HEADER-MERGE-NEXT: VirtualAddress: 0x2000
 
 # RUN: lld-link -safeseh:no /out:%t.dll /dll %t.obj /export:exportfn1,@5 \
 # RUN:   /export:exportfn2 /export:mangled
 # RUN: llvm-objdump -p %t.dll | FileCheck --check-prefix=CHECK2 %s
 
 # CHECK2:      Export Table:
-# CHECK2:      DLL name: export32.test.tmp.dll
-# CHECK2:      Ordinal      RVA  Name
-# CHECK2-NEXT:       1        0
-# CHECK2-NEXT:       2        0
-# CHECK2-NEXT:       3        0
-# CHECK2-NEXT:       4        0
+# CHECK2-NEXT: DLL name: export32.test.tmp.dll
+# CHECK2-NEXT: Ordinal base: 1
+# CHECK2-NEXT: Ordinal      RVA  Name
 # CHECK2-NEXT:       5   0x1008  exportfn1
 # CHECK2-NEXT:       6   0x1010  exportfn2
 # CHECK2-NEXT:       7   0x1010  exportfn3
 # CHECK2-NEXT:       8   0x1010  mangled
 
 # RUN: lld-link -safeseh:no /out:%t.dll /dll %t.obj /export:exportfn1,@5,noname /export:exportfn2
 # RUN: llvm-objdump -p %t.dll | FileCheck --check-prefix=CHECK3 %s
 
 # CHECK3:      Export Table:
-# CHECK3:      DLL name: export32.test.tmp.dll
-# CHECK3:      Ordinal      RVA  Name
-# CHECK3-NEXT:       1        0
-# CHECK3-NEXT:       2        0
-# CHECK3-NEXT:       3        0
-# CHECK3-NEXT:       4        0
+# CHECK3-NEXT: DLL name: export32.test.tmp.dll
+# CHECK3-NEXT: Ordinal base: 1
+# CHECK3-NEXT: Ordinal      RVA  Name
 # CHECK3-NEXT:       5   0x1008
 # CHECK3-NEXT:       6   0x1010  exportfn2
 
 # RUN: lld-link -safeseh:no /out:%t.dll /dll %t.obj /export:f1=exportfn1 /export:f2=exportfn2
 # RUN: llvm-objdump -p %t.dll | FileCheck --check-prefix=CHECK4 %s
 
 # CHECK4:      Export Table:
-# CHECK4:      DLL name: export32.test.tmp.dll
-# CHECK4:      Ordinal      RVA  Name
+# CHECK4-NEXT: DLL name: export32.test.tmp.dll
+# CHECK4-NEXT: Ordinal base: 1
+# CHECK4-NEXT: Ordinal      RVA  Name
 # CHECK4-NEXT:       1   0x1010  exportfn3
 # CHECK4-NEXT:       2   0x1008  f1
 # CHECK4-NEXT:       3   0x1010  f2
 
 # RUN: echo "EXPORTS exportfn1 @3" > %t.def
 # RUN: echo "fn2=exportfn2 @2" >> %t.def
 # RUN: lld-link -safeseh:no /out:%t.dll /dll %t.obj /def:%t.def
 # RUN: llvm-objdump -p %t.dll | FileCheck --check-prefix=CHECK5 %s
 
 # RUN: echo "EXPORTS exportfn1 @ 3" > %t.def
 # RUN: echo "fn2=exportfn2 @ 2" >> %t.def
 # RUN: lld-link -safeseh:no /out:%t.dll /dll %t.obj /def:%t.def
 # RUN: llvm-objdump -p %t.dll | FileCheck --check-prefix=CHECK5 %s
 
 # CHECK5:      Export Table:
-# CHECK5:      DLL name: export32.test.tmp.dll
-# CHECK5:      Ordinal      RVA  Name
-# CHECK5-NEXT:       1        0
+# CHECK5-NEXT: DLL name: export32.test.tmp.dll
+# CHECK5-NEXT: Ordinal base: 1
+# CHECK5-NEXT: Ordinal      RVA  Name
 # CHECK5-NEXT:       2   0x1010  fn2
 # CHECK5-NEXT:       3   0x1008  exportfn1
 # CHECK5-NEXT:       4   0x1010  exportfn3
 
 # RUN: lld-link -safeseh:no /out:%t.dll /dll %t.obj /export:exportfn1 /export:exportfn2 \
 # RUN:   /export:exportfn1 /export:exportfn2,@5 >& %t.log
 # RUN: FileCheck -check-prefix=CHECK6 %s < %t.log
 
 # CHECK6:     duplicate /export option: _exportfn2
 # CHECK6-NOT: duplicate /export option: _exportfn1
 
 # RUN: lld-link -safeseh:no /out:%t.dll /dll %t.obj /export:foo=mangled
 # RUN: llvm-objdump -p %t.dll | FileCheck --check-prefix=CHECK7 %s
 
 # CHECK7:      Export Table:
-# CHECK7:      DLL name: export32.test.tmp.dll
-# CHECK7:      Ordinal      RVA  Name
+# CHECK7-NEXT: DLL name: export32.test.tmp.dll
+# CHECK7-NEXT: Ordinal base: 1
+# CHECK7-NEXT: Ordinal      RVA  Name
 # CHECK7-NEXT:       1   0x1010  exportfn3
 # CHECK7-NEXT:       2   0x1010  foo
 
 --- !COFF
 header:
   Machine:         IMAGE_FILE_MACHINE_I386
   Characteristics: []
 sections:
   - Name:            .text

llvm/tools/llvm-objdump/COFFDump.cpp

static void printExportTable(const COFFObjectFile *Obj) {
   if (I->getDllName(DllName))
     return;
   if (I->getOrdinalBase(OrdinalBase))
     return;
   outs() << " DLL name: " << DllName << "\n";
   outs() << " Ordinal base: " << OrdinalBase << "\n";
   outs() << " Ordinal      RVA  Name\n";
   for (; I != E; I = ++I) {
-    uint32_t Ordinal;
-    if (I->getOrdinal(Ordinal))
-      return;
     uint32_t RVA;
     if (I->getExportRVA(RVA))
       return;
+    StringRef Name;
+    if (I->getSymbolName(Name))
+      continue;
+    if (!RVA && Name.empty())
+      continue;
+
+    uint32_t Ordinal;
+    if (I->getOrdinal(Ordinal))
+      return;
     bool IsForwarder;
     if (I->isForwarder(IsForwarder))
       return;
 
     if (IsForwarder) {
       // Export table entries can be used to re-export symbols that
       // this COFF file is imported from some DLLs. This is rare.
       // In most cases IsForwarder is false.
       outs() << format("   %5d         ", Ordinal);
     } else {
       outs() << format("   %5d %# 8x", Ordinal, RVA);
     }
 
-    StringRef Name;
-    if (I->getSymbolName(Name))
-      continue;
     if (!Name.empty())
       outs() << "  " << Name;
     if (IsForwarder) {
       StringRef S;
       if (I->getForwardTo(S))
         return;
       outs() << " (forwarded to " << S << ")";
     }