[PATCH] D89241: [RTDYLD] be more defensive about relocations with empty symbol names

[Valentin Churavy via Phabricator via llvm-commits]

vchuravy created this revision.
vchuravy added a reviewer: lhames.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.
vchuravy requested review of this revision.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89241

Files:
  llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp


Index: llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
===================================================================
--- llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
+++ llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
@@ -308,6 +308,7 @@
                         << " SID: " << SectionID
                         << " Offset: " << format("%p", (uintptr_t)Addr)
                         << " flags: " << *FlagsOrErr << "\n");
+      assert(!Name.empty()); // Should not enter an empty symbol
       GlobalSymbolTable[Name] = SymbolTableEntry(SectionID, Addr, *JITSymFlags);
     } else if (SymType == object::SymbolRef::ST_Function ||
                SymType == object::SymbolRef::ST_Data ||
@@ -340,6 +341,7 @@
                         << " SID: " << SectionID
                         << " Offset: " << format("%p", (uintptr_t)SectOffset)
                         << " flags: " << *FlagsOrErr << "\n");
+      assert(!Name.empty()); // Should not enter an empty symbol
       GlobalSymbolTable[Name] =
           SymbolTableEntry(SectionID, SectOffset, *JITSymFlags);
     }
@@ -769,6 +771,7 @@
 
     LLVM_DEBUG(dbgs() << "Allocating common symbol " << Name << " address "
                       << format("%p", Addr) << "\n");
+    assert(!Name.empty()); // Should not enter an empty symbol
     GlobalSymbolTable[Name] =
         SymbolTableEntry(SectionID, Offset, std::move(*JITSymFlags));
     Offset += Size;
@@ -930,6 +933,7 @@
   if (Loc == GlobalSymbolTable.end()) {
     ExternalSymbolRelocations[SymbolName].push_back(RE);
   } else {
+    assert(!SymbolName.empty() && "Empty symbol should not be in GlobalSymbolTable");
     // Copy the RE since we want to modify its addend.
     RelocationEntry RECopy = RE;
     const auto &SymInfo = Loc->second;
@@ -1107,6 +1111,7 @@
         // New entries may have been added to the relocation list.
         i = ExternalSymbolRelocations.find(Name);
       } else {
+        assert(!Name.empty() && "Empty symbol should not be in GlobalSymbolTable");
         // We found the symbol in our global table.  It was probably in a
         // Module that we loaded previously.
         const auto &SymInfo = Loc->second;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D89241.297588.patch
Type: text/x-patch
Size: 2196 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201012/714576bb/attachment.bin>