Page MenuHomePhabricator

[RTDYLD] be more defensive about relocations with empty symbol names
AbandonedPublic

Authored by vchuravy on Oct 12 2020, 7:55 AM.

Details

Reviewers
lhames
Summary

I am unsure if we should add these asserts, so I am putting this up for discussions.

The impetus for this was https://github.com/JuliaLang/julia/pull/37842#issuecomment-706651005
IIUC we map many relocations to the symbol name "". (See https://github.com/llvm/llvm-project/blame/176249bd6732a8044d457092ed932768724a6f06/llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp#L1199)
and https://github.com/llvm/llvm-project/blob/176249bd6732a8044d457092ed932768724a6f06/llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp#L929 looks at GlobalSymbolTable to determine whether or not
this is an absolute relocation. If someone happens to insert a symbol that has a "" as the name, all the absolute relocations will now be treated wrong.

So my question is: We are using "" as a sentinel value for absolute relocations. Should we ensure that actually is a sentinel value, or is there a platform were "" is a valid symbol name,
and we should use a different sentinel, like the original NULL pointer?

For JuliaLang I am currently filterting out the wrong relocation, after I was unable to find the producer of it.

Diff Detail

Unit TestsFailed

TimeTest
3,970 mswindows > Clang-Unit.DirectoryWatcher/_/DirectoryWatcherTests_exe::DirectoryWatcherTest.AddFiles
Note: Google Test filter = DirectoryWatcherTest.AddFiles [==========] Running 1 test from 1 test case.
3,740 mswindows > Clang-Unit.DirectoryWatcher/_/DirectoryWatcherTests_exe::DirectoryWatcherTest.DeleteFile
Note: Google Test filter = DirectoryWatcherTest.DeleteFile [==========] Running 1 test from 1 test case.
3,790 mswindows > Clang-Unit.DirectoryWatcher/_/DirectoryWatcherTests_exe::DirectoryWatcherTest.ModifyFile
Note: Google Test filter = DirectoryWatcherTest.ModifyFile [==========] Running 1 test from 1 test case.

Event Timeline

vchuravy created this revision.Oct 12 2020, 7:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2020, 7:55 AM
vchuravy requested review of this revision.Oct 12 2020, 7:55 AM
vchuravy updated this revision to Diff 297590.Oct 12 2020, 7:58 AM

clang-format

vtjnash added a subscriber: vtjnash.Nov 3 2020, 7:30 AM

I think these need to be handled in these places, given how clang emits relocations in .dwarf sections. For some contextual history, this was added as a sentinel for absolute symbols in https://github.com/llvm/llvm-project/commit/ad6d349fbcb2f9ced3c57b5b61231315175bcb47 (and later adjusted in https://github.com/llvm/llvm-project/commit/8f1f87c73487dcb83cf5d6d44b062f1e9796fae6). If I compare readelf --relocations of clang vs. gcc, I see GCC emit a name for these symbols for x86 inside the .dwarf sections, but that clang uses the name NULL, which gets translated there into the empty string.

llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
311–312
344–346
1116

This is unreachable, since line 1092 already handled it (which I think is also the reason for needing the rest of this patch)

vchuravy abandoned this revision.Nov 3 2020, 2:51 PM