Take the Index into account in getDelayImportTable, otherwise we
always return the entry for the first delay DLL reference.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 30039 Build 30038: arc lint + arc unit
Event Timeline
Comment Actions
I'm not sure how to update the tests for this change. Ideally I'd change test/tools/llvm-readobj/imports.test to use an input that has multiple delay-load DLL references. The inputs for that are (binary) PE files test/tools/llvm-readobj/Inputs/imports.exe.coff-x86-64 and test/tools/llvm-readobj/Inputs/imports.exe.coff-i386. @ruiu , do we have sources for those binaries checked in or otherwise available somewhere? Should I just generate new ones with similar import lists?
Comment Actions
Can you tell me how to reproduce the issue? Maybe that's a good start to think about how to write a test.
Comment Actions
Sure, sorry.
1: Create a PE file with delay-load references to two DLLs:
>type Alpha.c
__declspec(dllexport) void one() {}
__declspec(dllexport) void two() {}
>type Beta.c
__declspec(dllexport) void left() { }
__declspec(dllexport) void right() { }
>type Root.c
__declspec(dllimport) void one();
__declspec(dllimport) void two();
__declspec(dllimport) void left();
__declspec(dllimport) void right();
int main(int argc, char** argv) {
one();
two();
left();
right();
return 0;
}
>cl /LD Alpha.c & cl /LD Beta.c & cl Root.c Alpha.lib Beta.lib delayimp.lib /link /delayload:Alpha.dll /delayload:Beta.dll- Invoke llvm-readobj --coff-imports on the exe produced in step 1.
Expected Results:
The "Delay Import" parts of the dump have different ModuleHandles, ImportAddressTables, ImportNameTables, and BoundDelayImportTables for the two DLL references
DelayImport {
Name: Alpha.dll
Attributes: 0x1
ModuleHandle: 0x17A30
ImportAddressTable: 0x17A00
ImportNameTable: 0x163B0
BoundDelayImportTable: 0x16400
UnloadDelayImportTable: 0x0
Import {
Symbol: two (1)
Address: 0x1400010B1
}
Import {
Symbol: one (0)
Address: 0x14000102C
}
}
DelayImport {
Name: Beta.dll
Attributes: 0x1
ModuleHandle: 0x17A38
ImportAddressTable: 0x17A18
ImportNameTable: 0x163C8
BoundDelayImportTable: 0x16418
UnloadDelayImportTable: 0x0
Import {
Symbol: right (1)
Address: 0x140001142
}
Import {
Symbol: left (0)
Address: 0x1400010BD
}
}Observed Results:
The "Delay Import" parts of the dump show the same ModuleHandles, ImportAddressTables, ImportNameTables, and BoundDelayImportTables for the two DLL references (repeating the values that are correct for the first one)
DelayImport {
Name: Alpha.dll
Attributes: 0x1
ModuleHandle: 0x17A30
ImportAddressTable: 0x17A00
ImportNameTable: 0x163B0
BoundDelayImportTable: 0x16400
UnloadDelayImportTable: 0x0
Import {
Symbol: two (1)
Address: 0x1400010B1
}
Import {
Symbol: one (0)
Address: 0x14000102C
}
}
DelayImport {
Name: Beta.dll
Attributes: 0x1
ModuleHandle: 0x17A30
ImportAddressTable: 0x17A00
ImportNameTable: 0x163B0
BoundDelayImportTable: 0x16400
UnloadDelayImportTable: 0x0
Import {
Symbol: right (1)
Address: 0x140001142
}
Import {
Symbol: left (0)
Address: 0x1400010BD
}
}Comment Actions
Thank you for your response. I think you need to create three YAML files, two files for DLLs and the other for the main program. Just like other test files, you convert yaml files to COFF object files, link them and verify the result. Did you see test files in lld/test/COFF? Please let me know if you have any question -- I can help you create test files. If you can't figure out how to create test files, I can create them for you, so please let me know if that's the case. Thanks.
Comment Actions
No, I hadn't seen the test files in lld/test/COFF; thanks for the pointers! Testcase added; I verified that it fails without the fix and passes with it, and that neither make check nor make check-lld regress.
Comment Actions
Generally looking good, but I'd make the test files smaller.
| lld/test/COFF/Inputs/delayimporttables-alpha.yaml | ||
|---|---|---|
| 10–33 ↗ | (On Diff #193551) | Remove this section. |
| 38–40 ↗ | (On Diff #193551) | Remove this section. |
| 42–53 ↗ | (On Diff #193551) | These symbols can be removed. |
| 66–77 ↗ | (On Diff #193551) | This can be removed. |
| 108–119 ↗ | (On Diff #193551) | I believe you don't need this symbol. |
| lld/test/COFF/delayimporttables.test | ||
| 1–3 ↗ | (On Diff #193551) | %t1, %t2, %t3 are more conventional than %t-alpha, beta, etc. |
Comment Actions
- Address review feedback
- Rename files to be more conventional, less Greek
- Remove superfluous sections
- Remove superfluous symbols
Comment Actions
A few more nits.
| lld/test/COFF/Inputs/delayimporttables-exe.yaml | ||
|---|---|---|
| 1 ↗ | (On Diff #193638) | You can move this contents to COFF/delayimporttables.test (and you probably should rename it delayimporttables.yaml) |
| 27–44 ↗ | (On Diff #193638) | I believe you can remove them. |
| 58–69 ↗ | (On Diff #193638) | This one as well. |
| 106–147 ↗ | (On Diff #193638) | Ditto. |
| lld/test/COFF/delayimporttables.test | ||
| 10 ↗ | (On Diff #193638) | Add a space after the first :. |
Comment Actions
LGTM
Thank you for doing this!
Do you have a commit access? If you don't have one, please let me know.