Fix: avoid creating unnecessary .got.plt section#1095
Conversation
| // Part of the eld Project, under the BSD License | ||
| // See https://github.com/qualcomm/eld/LICENSE.txt for license information. | ||
| // SPDX-License-Identifier: BSD-3-Clause | ||
| //===----------------------------------------------------------------------===// |
There was a problem hiding this comment.
Remove this license from test file.
| // If we are creating a GOT, always create a .got.plt. | ||
| if (!getGOTPLT()->hasFragments()) { | ||
| // Only create .got.plt when dynamic linking is involved | ||
| if (!getGOTPLT()->hasFragments() && config().isCodeDynamic()) { |
There was a problem hiding this comment.
What about if you have a IFUNC ?
|
A few existing I feel that the test is too narrow. It could also cover static PIE executables and include a a positive check that dynamic/shared/pie still create I also suspect that the test as written might not fail even if we did still emit an unnecessary A For ARM and Hexagon, We duplicate code across backends with this change. Is there a way to share it? |
| if (!getGOTPLT()->hasFragments()) { | ||
| // Only create .got.plt when dynamic linking is involved | ||
| if (!getGOTPLT()->hasFragments() && config().isCodeDynamic()) { | ||
| // TODO: This should be GOT0, not GOTPLT0. |
There was a problem hiding this comment.
The true fix, which might be outside the scope of this PR, would be to address this TODO: createGOT() creates the GOT's reserved slot as GOTPLT0 in .got.plt instead of as a GOT0 in .got. It looks like RISCV has already partially done the fix.
There was a problem hiding this comment.
Hi @quic-areg , Thank you for the insight! I am currently investigating the IFUNC case to make sure the fix handles it correctly.
Thank You ::))
deepakshirkem
force-pushed
the
fix/unnecessary-got-plt-1030
branch
from
903095d to
282289b
Compare
|
Hi @quic-areg @quic-seaswara, I have updated the fix to correctly handle the IFUNC case. The condition now creates GOTPLT0 when:
I have tested the following cases locally:
Could you please run the pipeline when you get a chance? Some test cases are unsupported in my local environment Thank you ::)) |
|
Hi @parth-07 / @quic-seaswara / @quic-areg , I just wanted to follow up on this PR when you get a chance. Just wanted to make sure this didn't get lost! Thank You :: |
ELD was unnecessarily creating .got.plt section when only .got section was required for static executables. The .got.plt section is only needed when dynamic linking is involved. Fix by checking isCodeDynamic() before creating GOTPLT0 entry in createGOT(). This fix is applied to all targets: AArch64, ARM, RISCV, Hexagon, x86_64 Signed-off-by: deepakshirkem <deepakshirke509@gmail.com>
deepakshirkem
force-pushed
the
fix/unnecessary-got-plt-1030
branch
from
282289b to
95996c1
Compare
|
Hi @quic-seaswara / @quic-areg , Please review the changes and If get chance run the pipeline. |
3 participants
Description
Problem
Fixes #1030
ELD unnecessarily creates
.got.pltsection when only.gotsection is required for a static executable.Fix
The
.got.pltsection is only needed when dynamic linking is involved. AddedisCodeDynamic()check before creatingGOTPLT0entry increateGOT()across all targets:Testing
Added test
UnnecessaryGOTPLTthat verifies.got.pltis not created for static executables that only require.got.cc @quic-seaswara @parth-07