[PATCH] D149611: [LLD][COFF] Allow overwriting directives exports with cmd-line exports

Tue May 2 01:41:53 PDT 2023

mstorsjo accepted this revision.
mstorsjo added a comment.
This revision is now accepted and ready to land.

Overall LGTM on this, a couple minor comments on the test.

================
Comment at: lld/test/COFF/ordinals-override.test:65
+    Alignment:       1
+    SectionData:     2020202F6578706F72743A62617A3D3F62617A4040594158585A202F44454641554C544C49423A224C4942434D5422202F44454641554C544C49423A224F4C444E414D455322202F4558504F52543A3F666F6F4040594158585A202F4558504F52543A3F6261724040594158585A20
+  - Name:            '.debug$S'
----------------
Can you add a comment what the contained directive string is?

================
Comment at: lld/test/COFF/ordinals-override.test:66
+    SectionData:     2020202F6578706F72743A62617A3D3F62617A4040594158585A202F44454641554C544C49423A224C4942434D5422202F44454641554C544C49423A224F4C444E414D455322202F4558504F52543A3F666F6F4040594158585A202F4558504F52543A3F6261724040594158585A20
+  - Name:            '.debug$S'
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
----------------
I guess the debug chunk is irrelevant for the test and could be left out?

================
Comment at: lld/test/COFF/ordinals-override.test:110
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+  - Name:            '@vol.md'
+    Value:           2
----------------
Symbols like this also probably could be left out here, keeping the testcase yaml more minimal?

(For tests like these, we usually start with handcrafted files - or strip down the compiler generated ones as far as possible.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149611