This patch adds support for creating Guard Address-Taken IAT Entry Tables (.giats$y sections) in object files, matching the behavior of MSVC. These contain lists of address-taken imported functions, which are used by the linker to create the final GIATS table.
Additionally, if any DLLs are delay-loaded, the linker must look through the .giats tables and add the respective load thunks of address-taken imports to the GFIDS table, as these are also valid call targets.
Details
- Reviewers
rnk ruiu amccarth theraven jhenderson hans aeubanks dmajor - Commits
- rG0139c8af8da7: [CFGuard] Add address-taken IAT tables and delay-load support
rGcfd8481da1ad: Reland [CFGuard] Add address-taken IAT tables and delay-load support
rGef4e971e5e18: [CFGuard] Add address-taken IAT tables and delay-load support
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
lld/COFF/Writer.cpp | ||
---|---|---|
1683 | Can we set either ProtectDelayLoadIAT or DelayLoadIATSection now, or does that take more work? | |
1762 | These two methods are essentially the same up until here. Your version seems more general. Can you rewrite markSymbolsForRVATable in terms of this utility? This code shouldn't be hot, I don't think we need to worry about the cost of an extra temporary vector of address-taken symbols. |
Rewrite markSymbolsForRVATable() in terms of getSymbolsFromSections()
lld/COFF/Writer.cpp | ||
---|---|---|
1683 | I think this will take more work (e.g. the latter requires moving the Delayload import table into its own .didat section). |
I believe this is causing https://crbug.com/1134215:
ninja -t msvc -e environment.x86 -- ..\..\third_party\llvm-build\Release+Asserts\bin\lld-link.exe /nologo -libpath:..\..\third_party\llvm-build\Release+Asserts\lib\clang\12.0.0\lib\windows -libpath:..\..\third_party\depot_tools\win_toolchain\vs_files\a687d8e2e4114d9015eb550e1b156af21381faac\VC\Tools\MSVC\14.26.28801\lib\x86 -libpath:..\..\third_party\depot_tools\win_toolchain\vs_files\a687d8e2e4114d9015eb550e1b156af21381faac\win_sdk\Lib\10.0.19041.0\um\x86 -libpath:..\..\third_party\depot_tools\win_toolchain\vs_files\a687d8e2e4114d9015eb550e1b156af21381faac\win_sdk\Lib\10.0.19041.0\ucrt\x86 -libpath:..\..\third_party\depot_tools\win_toolchain\vs_files\a687d8e2e4114d9015eb550e1b156af21381faac\VC\Tools\MSVC\14.26.28801\atlmfc\lib\x86 /IMPLIB:./browser_switcher_bho.dll.lib /DLL /OUT:./browser_switcher_bho.dll /PDB:./browser_switcher_bho.dll.pdb @./browser_switcher_bho.dll.rsp unknown symbol kind UNREACHABLE executed at C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Symbols.h:443! PLEASE submit a bug report to https://crbug.com and run tools/clang/scripts/process_crashreports.py (only works inside Google) which will upload a report and include the crash backtrace. Stack dump: 0. Program arguments: ..\..\third_party\llvm-build\Release+Asserts\bin\lld-link.exe /nologo -libpath:..\..\third_party\llvm-build\Release+Asserts\lib\clang\12.0.0\lib\windows -libpath:..\..\third_party\depot_tools\win_toolchain\vs_files\a687d8e2e4114d9015eb550e1b156af21381faac\VC\Tools\MSVC\14.26.28801\lib\x86 -libpath:..\..\third_party\depot_tools\win_toolchain\vs_files\a687d8e2e4114d9015eb550e1b156af21381faac\win_sdk\Lib\10.0.19041.0\um\x86 -libpath:..\..\third_party\depot_tools\win_toolchain\vs_files\a687d8e2e4114d9015eb550e1b156af21381faac\win_sdk\Lib\10.0.19041.0\ucrt\x86 -libpath:..\..\third_party\depot_tools\win_toolchain\vs_files\a687d8e2e4114d9015eb550e1b156af21381faac\VC\Tools\MSVC\14.26.28801\atlmfc\lib\x86 /IMPLIB:./browser_switcher_bho.dll.lib /DLL /OUT:./browser_switcher_bho.dll /PDB:./browser_switcher_bho.dll.pdb @./browser_switcher_bho.dll.rsp #0 0x00007ff7471970b5 HandleAbort C:\b\s\w\ir\cache\builder\src\third_party\llvm\llvm\lib\Support\Windows\Signals.inc:408:0 #1 0x00007ff7495b0063 raise minkernel\crts\ucrt\src\appcrt\misc\signal.cpp:547:0 #2 0x00007ff7495aa9d0 abort minkernel\crts\ucrt\src\appcrt\startup\abort.cpp:71:0 #3 0x00007ff74716d416 llvm::llvm_unreachable_internal(char const *, char const *, unsigned int) C:\b\s\w\ir\cache\builder\src\third_party\llvm\llvm\lib\Support\ErrorHandling.cpp:213:0 #4 0x00007ff7471f067c lld::coff::Defined::getChunk(void) C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Symbols.h:443:0 #5 0x00007ff747244292 addSymbolToRVASet C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Writer.cpp:1542:0 #6 0x00007ff747245187 `anonymous namespace'::Writer::createGuardCFTables C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Writer.cpp:1655:0 #7 0x00007ff747246283 `anonymous namespace'::Writer::createMiscChunks C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Writer.cpp:988:0 #8 0x00007ff74724bbb8 `anonymous namespace'::Writer::run C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Writer.cpp:614:0 #9 0x00007ff74724d439 lld::coff::writeResult(void) C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Writer.cpp:311:0 #10 0x00007ff7471f852a lld::coff::LinkerDriver::link(class llvm::ArrayRef<char const *>) C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Driver.cpp:2143:0 #11 0x00007ff7471f8af0 lld::coff::link(class llvm::ArrayRef<char const *>, bool, class llvm::raw_ostream &, class llvm::raw_ostream &) C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Driver.cpp:96:0 #12 0x00007ff747158898 lldMain C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\tools\lld\lld.cpp:155:0 #13 0x00007ff747158ce7 main C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\tools\lld\lld.cpp:211:0 #14 0x00007ff749583a04 __scrt_common_main_seh d:\A01\_work\6\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288:0 #15 0x00007ff9c0982774 (C:\Windows\System32\KERNEL32.DLL+0x12774) #16 0x00007ff9c0a90d51 (C:\Windows\SYSTEM32\ntdll.dll+0x70d51)
Printing out the kind() of the failing symbol, it gives 40, which is nowhere in the range of Symbol::Kind. Reverting for now, let me know if you need more details for fixing this.
We don't normally revert commits for downstream build failures. Please can you submit a test case that triggers this issue?
Whoops, forgot that was on top of this patch. Commented inline in this patch
lld/COFF/Symbols.h | ||
---|---|---|
352 | this needs to be initialized |
Thanks for the diagnostics @aeubanks! Does this now resolve the issue with the Chrome build?
When I was looking into it, after initializing loadThunkSym to nullptr, the build step that was failing then succeeded. So that should be good enough.
This caused a regression when calling through function pointers in a certain way as seen in sqlite3: https://bugs.llvm.org/show_bug.cgi?id=47905
Correctly handle __imp_ symbols in .giats and .gfids sections
For address-taken dllimport functions with a corresponding __imp_ symbol defined, add the __imp_ symbol to the .giats section. Always add the symbols of address-taken functions to the .gfids section.
@hans and @dmajor thanks for reporting this issue (https://bugs.llvm.org/show_bug.cgi?id=47905).
Do the latest changes resolve this in Chromium and sqlite3?
FYI: While the reland didn't break anything, it apparently also didn't fix everything it set out to fix, see https://bugs.chromium.org/p/chromium/issues/detail?id=1132179#c13 and onwards.
this needs to be initialized