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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

In D149610#4315001 <https://reviews.llvm.org/D149610#4315001>, @alvinhochun wrote:

> 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.

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



================
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
----------------
alvinhochun wrote:
> 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 did it in the other patch, but I could add it here earlier.


================
Comment at: llvm/lib/Object/COFFObjectFile.cpp:1568-1570
+  Result = (bool)E;
+  consumeError(std::move(E));
+  return Error::success();
----------------
jhenderson wrote:
> 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?
I ended up removing this code, see other comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149610