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

[Alvin Wong via Phabricator via llvm-commits]

alvinhochun added a comment.

In D149610#4312974 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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->getSymbolName`earlier so you can check the RVA and the name before printing the entry.



================
Comment at: lld/test/COFF/export.test:17-18
 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:       5   0x1008  exportfn1
 CHECK2-NEXT:       6   0x1010  exportfn2
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149610