[BOLT] Fix plt relocations symbol match

[yota9]

Summary

The BFD linker adds the symbol versioning string to the symbol name in symtab.
Skip the versioning part in order to find the registered PLT function.

Vladislav Khmelevsky,
Advanced Software Technology Lab, Huawei

[Amir]

Can you please follow the example of compiler-rt to check that bfd is available on the test system and only enable the new test if it is?
Check here: compiler-rt/cmake/config-ix.cmake: GNU_LD_EXECUTABLE
And here: compiler-rt/test/lit.common.cfg.py: config.available_features.add('binutils_lto')

bolt/lib/Rewrite/RewriteInstance.cpp

1890

Can you please add an example of a full symbol name with versioning?

bolt/test/runtime/AArch64/plt.c

5

Can you please keep lld here and add invocation with bfd below as an extra test?

[yota9]

bolt/lib/Rewrite/RewriteInstance.cpp

1890

Sure

[yota9]

Add gnu_ld check + separate test

[maksfb]

bolt/test/runtime/plt_gnu_ld.test

10 ↗

(On Diff #417291)

Hi @yota9, could you please make the checks for properly updated PLT explicit instead of relying on executing the binary?

[maksfb]

bolt/lib/Rewrite/RewriteInstance.cpp

1886–1896

Could you please factor out the code above? Why is it AArch64-specific?

[yota9]

bolt/lib/Rewrite/RewriteInstance.cpp

1886–1896

Sorry you want me to remove IsAArch64 check? I didn't check it on X86. AFAIU there is no need for x86 to have this code since this is used for aarch64 because there are instruction pairs like adrp + add, which give us the final symbol value in pair, but the X86 may use extracted value for single instruction

bolt/test/runtime/plt_gnu_ld.test

10 ↗

(On Diff #417291)

I will try t

10 ↗

(On Diff #417291)

Sorry, something went wrong here :) I will try to do it soon :)

[yota9]

Update tests, move PLT symbol search code to separate function

[maksfb]

Summary: s/linkeradds/linker adds/ and s/symbtab/symtab/

bolt/include/bolt/Core/BinaryContext.h

779 ↗

(On Diff #418207)

nit: could you use PLT (all caps) in function names to be consistent with the rest of the code in BOLT?

[yota9]

bolt/include/bolt/Core/BinaryContext.h

779 ↗

(On Diff #418207)

Yes, sure, I always forget about PLT.. :D

[yota9]

Address comments

[maksfb]

@yota9, regarding the tests - will we be able to detect the change in behavior on non-aarch64 (x86) platform? I.e. will any of the tests fail without the fix?

[yota9]

@maksfb I see your point. From the one hand we have a fix. From the other hand the tests are common for x86 and aarch64, i.e. it doesn't check the fix, it checks the overall possibility to work with PLT properly. Ideally we should have aarch64 machine available for testing, we've got this task in the backlog and Elvina should start investigation on aarch64 buildbot soon. As for now I would like these runtime tests to exist, but I will add one more non-runtime aarch64-specific test (that is using BOLT asm dump) so we will be able to check it on non-arm platform

[maksfb]

Great. Adding non-runtime test sounds good to me.

[yota9]

Add non-runtime tests

[maksfb]

@yota9, sorry, AArch64/plt_gnu_ld.test wouldn't build on x86. Could you use YAML and yaml2obj for the test?

[yota9]

Add BFD linked yaml test file

[yota9]

In D122039#3417876, @maksfb wrote:

@yota9, sorry, AArch64/plt_gnu_ld.test wouldn't build on x86. Could you use YAML and yaml2obj for the test?

Done :)

[maksfb]

Could you please rename test files to use "-" instead of "_"?

bolt/test/AArch64/plt_gnu_ld.test

7 ↗

(On Diff #419557)

I tried that fix locally and got "no PT_LOAD pheader seen" error.

bolt/test/AArch64/plt_lld.test

4 ↗

(On Diff #419557)

plt.c will not cross-compile since it includes header files.

[yota9]

bolt/test/AArch64/plt_gnu_ld.test

7 ↗

(On Diff #419557)

The old file was remained on the pass, thanks for the remark.

[yota9]

@maksfb Thanks done

[maksfb]

AArch64/plt-lld.test is still broken.

[yota9]

@maksfb I don't see the reason, everything works for me. Do you use this patch with your aarch64 tests changes? The nostdlib flag should be passed from your patch, I don't see any other problem to run the test on x86.

[yota9]

@maksfb You were right, after rebase on your patch I've got the problems too.
I was sure the undefined symbols with unresolved-symbols=ignore-all option will create PLT entry by using symbol name, but it does not.
Anyway we will just create stub library now for the testing purposes and the problem will gone.

[yota9]

nit header protection

[yota9]

Rebase

bolt/lib/Rewrite/RewriteInstance.cpp

bool RewriteInstance::analyzeRelocation(
   } else {
     const SymbolRef &Symbol = *SymbolIter;
     SymbolName = std::string(cantFail(Symbol.getName()));
     SymbolAddress = cantFail(Symbol.getAddress());
     SkipVerification = (cantFail(Symbol.getType()) == SymbolRef::ST_Other);
     // Section symbols are marked as ST_Debug.
     IsSectionRelocation = (cantFail(Symbol.getType()) == SymbolRef::ST_Debug);
     // Check for PLT entry registered with symbol name
     if (!SymbolAddress && IsAArch64) {
       BinaryData *BD = BC->getBinaryDataByName(SymbolName + "@PLT");
+      size_t End;
+      if ((!BD) && ((End = SymbolName.find("@")) != std::string::npos)) {
+        // The symbol might contain its versioning in name
+        SymbolName = SymbolName.substr(0, End);
+        BD = BC->getBinaryDataByName(SymbolName + "@PLT");
+      }
+
       SymbolAddress = BD ? BD->getAddress() : 0;
     }
   }
   // For PIE or dynamic libs, the linker may choose not to put the relocation
   // result at the address if it is a X86_64_64 one because it will emit a
   // dynamic relocation (X86_RELATIVE) for the dynamic linker and loader to
   // resolve it at run time. The static relocation result goes as the addend
   // of the dynamic relocation in this case. We can't verify these cases.
   // FIXME: perhaps we can try to find if it really emitted a corresponding
   // RELATIVE relocation at this offset with the correct value as the addend.

bolt/test/runtime/AArch64/plt.c

 // This test checks that the pointers to PLT are properly updated.
+// Use bfd linker since it may add versioning string to the symbol
+// names in symbtab.
 
-// RUN: %clang %cflags %s -fuse-ld=lld \
+// RUN: %clang %cflags %s -fuse-ld=bfd \
 // RUN:    -o %t.exe -Wl,-q
 // RUN: llvm-bolt %t.exe -o %t.bolt.exe -use-old-text=0 -lite=0
 // RUN: %t.bolt.exe
 
 #include <string.h>
 
 void *(*memcpy_p)(void *dest, const void *src, size_t n);
 void *(*memset_p)(void *dest, int c, size_t n);