This is an archive of the discontinued LLVM Phabricator instance.

[COFF] Fix delay import directory iterator
ClosedPublic

Authored by JosephTremoulet on Apr 1 2019, 11:11 AM.

Details

Summary

Take the Index into account in getDelayImportTable, otherwise we
always return the entry for the first delay DLL reference.

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2019, 11:11 AM
JosephTremoulet added a comment.EditedApr 1 2019, 11:13 AM

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?

ruiu added a comment.Apr 1 2019, 9:34 PM

Can you tell me how to reproduce the issue? Maybe that's a good start to think about how to write a test.

Can you tell me how to reproduce the issue? Maybe that's a good start to think about how to write a test.

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
  1. 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
  }
}
ruiu added a comment.Apr 3 2019, 2:10 AM

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.

  • add testcase
JosephTremoulet added a comment.EditedApr 3 2019, 11:16 AM

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.

ruiu added a comment.Apr 3 2019, 4:53 PM

Generally looking good, but I'd make the test files smaller.

lld/test/COFF/Inputs/delayimporttables-alpha.yaml
10–33

Remove this section.

38–40

Remove this section.

42–53

These symbols can be removed.

66–77

This can be removed.

108–119

I believe you don't need this symbol.

lld/test/COFF/delayimporttables.test
1–3

%t1, %t2, %t3 are more conventional than %t-alpha, beta, etc.

  • Address review feedback
    • Rename files to be more conventional, less Greek
    • Remove superfluous sections
    • Remove superfluous symbols
JosephTremoulet marked 5 inline comments as done.Apr 3 2019, 5:48 PM

Names normalized, cruft removed, thanks for the feedback.

ruiu added a comment.Apr 3 2019, 5:55 PM

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
11

Add a space after the first :.

  • Merge exe yaml with test file, remove more cruft
JosephTremoulet marked 6 inline comments as done.Apr 3 2019, 6:11 PM

Updated.

ruiu accepted this revision.Apr 3 2019, 6:16 PM

LGTM

Thank you for doing this!

Do you have a commit access? If you don't have one, please let me know.

This revision is now accepted and ready to land.Apr 3 2019, 6:16 PM

Thanks for the review!

Yes I have commit access.

This revision was automatically updated to reflect the committed changes.