Ditto. dumpbin /exports and binutils objdump do skip empty entries as well. This patch is to simplify an upcoming change in LLD.
This change doesn't seem consistent and/or directly explainable by the change itself?
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.
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?
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?
@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.
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  +base 1000 Export RVA  +base 1020 Export RVA [Ordinal/Name Pointer] Table [ 0] ?bar@@YAXXZ  ?foo@@YAXXZ  baz
The previous pattern was wrong. The RVA 0 was actually matching the first character of 0x1010.
I was planning to commit it separately as a NFC patch, is that ok with you?
|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?
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'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.
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
I did it in the other patch, but I could add it here earlier.
|1568–1570 ↗||(On Diff #518530)|
I ended up removing this code, see other comment.