[X86_64] Emit _GLOBAL_OFFSET_TABLE_ symbol for x86-64#1123
[X86_64] Emit _GLOBAL_OFFSET_TABLE_ symbol for x86-64#1123deepakshirkem wants to merge 1 commit into
Conversation
| 0x0, // value | ||
| FragmentRef::null(), ResolveInfo::Hidden); | ||
| if (m_pGOTSymbol) | ||
| m_pGOTSymbol->setShouldIgnore(false); |
There was a problem hiding this comment.
Can you check the behavior of bfd and lld that we are doing the same ?
| @@ -0,0 +1,11 @@ | |||
| #----------GlobalOffsetTable.test--------------------- Executable ------# | |||
| #BEGIN_COMMENT | |||
There was a problem hiding this comment.
Does this test make sense to be part of Common ?
Can you add a test with linker script with GOT/GOT.PLT discarded and see the behavior ?
There was a problem hiding this comment.
Moved to Common/standalone/GlobalOffsetTable/
Added Common/standalone/GlobalOffsetTableDiscard/
|
|
||
| int *v = &_GLOBAL_OFFSET_TABLE_; | ||
|
|
||
| int a; |
4ad481e to
b8e1b80
Compare
|
Hi @quic-seaswara, Please go one more time. |
b8e1b80 to
f0726dc
Compare
|
Hi @parth-07, Please review these changes when you get a chance? Also, if possible, could you re-run the pipeline? |
There was a problem hiding this comment.
What should be the value of _GLOBAL_OFFSET_TABLE_ when there is no .got.plt section?
Example:
#!/usr/bin/env bash
cat >1.c <<\EOF
extern int _GLOBAL_OFFSET_TABLE_;
int foo() { return 1; }
int bar() { return 3; }
int *u = &_GLOBAL_OFFSET_TABLE_;
EOF
LDs=(ld.eld ld.lld ld.bfd)
SFs=(eld lld bfd)
clang -o 1.o -target x86_64-linux-gnu 1.c -c -fPIC
for i in "${!SFs[@]}"; do
${LDs[$i]} -o lib1.${SFs[$i]}.so 1.o -shared
done
For this example, eld shared library has _GLOBAL_OFFSET_TABLE_ defined to 0 whereas lld and bfd have valid GLOBAL_OFFSET_TABLE_. Can you please look into it?
Additionally, we need to document the behavior for the properties(local, default vs hidden visibility, NOTYPE vs OBJECT ...) of the _GLOBAL_OFFSET_TABLE_ symbol.
f0726dc to
8981a64
Compare
|
Hi @parth-07, Please go one more time. |
| RUN: %link %linkopts %t1.o -o %t1.so -shared 2>&1 | ||
| RUN: %readelf -s %t1.so | %filecheck %s | ||
| #END_TEST | ||
| CHECK: {{[1-9][0-9a-f]+}}{{.*}}OBJECT{{.*}}LOCAL{{.*}}HIDDEN{{.*}}_GLOBAL_OFFSET_TABLE_ |
There was a problem hiding this comment.
it would be great if the test can be improved to point to the section as intended.
8981a64 to
d0788aa
Compare
|
Hi @quic-seaswara, test has been updated. Please take a look? |
@deepakshirkem can you add a test for this case ? |
d0788aa to
4263a92
Compare
|
Hi @quic-seaswara, Can you please re-run the pipeline. I forgot to add specific target on that test. |
| # relocations. ELD was incorrectly emitting value 0x0 in this case. | ||
| #END_COMMENT | ||
| #START_TEST | ||
| REQUIRES: x86 |
There was a problem hiding this comment.
move REQUIRES to the top.
| #START_TEST | ||
| REQUIRES: x86 | ||
| RUN: %clang %clangopts -c %p/Inputs/1.c -o %t1.o -fPIC -ffunction-sections | ||
| RUN: %link %linkopts %t1.o -o %t1.so -shared 2>&1 |
There was a problem hiding this comment.
What if it is a non shared library link, can we also test for -pie ?
There is no .got.plt so what does GLOBAL_OFFSET_TABLE point to ?
There was a problem hiding this comment.
Hi @quic-seaswara, Thank you. I tested both the -pie and shared both are correctly point .got.plt. Adding new test for -pie.
quic-areg
left a comment
There was a problem hiding this comment.
The tests should live under test/X86, not test/Common
|
Hi @quic-areg, Moving all test in test/X86 |
4263a92 to
fc3f5ff
Compare
|
Hi @quic-seaswara / @quic-areg, Please take another look when you get a chance? Thank you. |
| @@ -0,0 +1,14 @@ | |||
| REQUIRES: x86 | |||
There was a problem hiding this comment.
Remove REQUIRES: x86 and move it to Common ?
There was a problem hiding this comment.
Hi @quic-seaswara, should I move only GlobalOffsetTableShared to Common, or all the tests we created? Also, if I remove REQUIRES: x86, this test will fail in CI because the fix is currently only implemented in x86_64LDBackend.cpp.
|
Hi @quic-areg / @quic-seaswara , Can you go one more time. Please re-run the pipeline whenever you get chance. |
| RUN: %clang %clangopts -c %p/Inputs/1.c -o %t1.o -fPIC -ffunction-sections | ||
| RUN: %not %link %linkopts -T %p/Inputs/discard.t %t1.o -o %t1.out 2>&1 | %filecheck %s | ||
| #END_TEST | ||
| CHECK: symbol '_GLOBAL_OFFSET_TABLE_' referenced |
There was a problem hiding this comment.
What is the error message emitted by eld in this case? Can you please add that as part of the test?
| RUN: %link %linkopts %t1.o -o %t1.out 2>&1 | ||
| RUN: %readelf -S -W -s %t1.out 2>&1 | %filecheck %s | ||
| #END_TEST | ||
| CHECK: .got.plt PROGBITS [[ADDR:[xa-f0-9]+]] |
There was a problem hiding this comment.
Can you please add a check in the test that makes sure that the ADDR is a non-zero value?
367c856 to
79ffa69
Compare
|
Hi @parth-07, Please go one more time. |
|
Hi @parth-07 / @quic-areg / @quic-seaswara, if you get a chance today to review these changes, please take a look. |
79ffa69 to
0e8fe39
Compare
|
Hi @quic-seaswara, Can you please run this pipeline one more time. |
|
Hi @quic-seaswara, The tests in |
82677bf to
03bf842
Compare
parth-07
left a comment
There was a problem hiding this comment.
Why are we using a new directory test/X86 instead of using the already existing directory test/x86_64?
03bf842 to
6d8ec8b
Compare
|
Hi @quic-seaswara / @parth-07 / @quic-areg, Please review one more time and re- run the pipeline. |
c08a053 to
2031466
Compare
2031466 to
ac692a3
Compare
|
Hi @quic-seaswara, Check the updated commit? I have removed the duplicate code. Please run the pipeline? I want to confirm the test works correctly with Hexagon and AArch64. |
Before you submit the code, you can locally run ninja check-eld on your machine to verify than waiting for us to trigger the runs. Let me know if that would work for you. |
ac692a3 to
54bd0b7
Compare
|
Yes, Will work. Thank You ::(( |
| RUN: %link %linkopts %t1.o -o %t1.out 2>&1 | ||
| RUN: %readelf -S -W -s %t1.out 2>&1 | %filecheck %s | ||
| #END_TEST | ||
| CHECK: .got.plt PROGBITS [[ADDR:0*[1-9a-f][0-9a-f]*]] |
There was a problem hiding this comment.
Can we have a common test for all architectures instead of duplicating test per architecture?
One way to do this is to define a lit substitution GlobalOffsetTableSection, and then pass that substitution to
FileCheck using -DGlobalOffsetTableSection=%GlobalOffsetTableSection, then this section can be referred in the CHECKs as [[GlobalOffsetTableSection]]
parth-07
left a comment
There was a problem hiding this comment.
We should remove the older tests (such as test/RISCV/standalone/GlobalOffsetTable/GlobalOffsetTable.test) which are redundant/duplicate now.
ELD did not emit _GLOBAL_OFFSET_TABLE_ for x86-64, causing an undefined
reference error when object files reference this symbol.
Add _GLOBAL_OFFSET_TABLE_ support for x86-64 and refactor the common
implementation into GNULDBackend base class:
- GNULDBackend::defineGlobalOffsetTableSymbol(): new helper that defines
_GLOBAL_OFFSET_TABLE_ with consistent properties across all targets:
Type: Object - consistent with ARM ELD and BFD
Binding: Local - consistent with ARM ELD and BFD
Visibility: Hidden - consistent with ARM ELD and LLD
- x86_64LDBackend::initTargetSymbols(): use helper instead of duplicated
addSymbol block.
- x86_64LDBackend::defineGOTSymbol(): bind symbol to first .got.plt
fragment so it points to the GOT base address.
- x86_64LDBackend::finalizeScanRelocations(): call defineGOTSymbol()
after scan and report an error if .got.plt is discarded but the
symbol is referenced, consistent with GNU ld behavior.
Refactor ARM, AArch64, Hexagon backends to use the same helper,
eliminating duplicated code. Add _GLOBAL_OFFSET_TABLE_ to RISCV
backend which was missing it entirely.
Move tests from x86_64/standalone/ to Common/standalone/ following
the DynamicGOTPLT pattern. Add per-target test variants for ARM,
AArch64, Hexagon, and RISCV covering executable, shared library,
PIE, and discard error cases.
Fixes qualcomm#570.
Signed-off-by: deepakshirkem <deepakshirke509@gmail.com>
54bd0b7 to
c8d298d
Compare
|
Hi @quic-seaswara, Any suggestions. |
Problem
ELD does not emit the GLOBAL_OFFSET_TABLE symbol for x86-64, causing an undefined reference error when object files reference this symbol. This affects code compiled with -fPIC or using the large code model.
Fix
Add GLOBAL_OFFSET_TABLE symbol support to the x86-64 backend following the same pattern used by other backends:
initTargetSymbols():Register the symbol as AsReferred so it is only defined if actually referenced by input files.defineGOTSymbol():Bind the symbol to the first fragment of .got.plt so it points to the GOT base address.finalizeScanRelocations():Call defineGOTSymbol() after all relocations have been scanned and .got.plt is populated.Testing
Added test/X86/standalone/GlobalOffsetTable/GlobalOffsetTable.test that verifies GLOBAL_OFFSET_TABLE is emitted and points to .got.plt.
Fixes #570.
cc @quic-seaswara