test: add tests and docs for section address linker options#1145
test: add tests and docs for section address linker options#1145sai18022001 wants to merge 4 commits into
Conversation
- Add tests for --section-start, -Ttext, -Tdata, -Tbss, -Ttext-segment - Add -Trodata-segment and -Tldata-segment support and tests - Update userguide with option descriptions Resolves qualcomm#1099 Signed-off-by: Sai Sanjay Chikne <saichikne180201@gmail.com>
9ac24b2 to
3eba23f
Compare
|
|
||
| uint64_t GNULDBackend::getImageBase(bool HasInterp, bool LoadEHdr) const { | ||
| if (auto TextSegment = config().options().textSegment()) | ||
| return *TextSegment; |
There was a problem hiding this comment.
Does -Ttext-segment override linker script assignment ?
There was a problem hiding this comment.
I tested this, when both -Ttext-segment and a linker script address assignment are specified, the linker script takes precedence. Is this the expected behavior, or should -Ttext-segment always override the linker script ?
There was a problem hiding this comment.
Pinging on this - wanted to confirm the expected behavior before making any further changes to GNULDBackend.cpp.
Based on GNU ld behavior, the linker script takes precedence over -Ttext-segment when an explicit address is assigned in the script. The current implementation follows this behavior. Please let me know if a different behavior is expected for ELD.
There was a problem hiding this comment.
Thanks for confirming. Please go ahead.
|
|
||
| void setImageBase(uint64_t Value) { ImageBase = Value; } | ||
|
|
||
| const std::optional<uint64_t> &textSegment() const { return TextSegment; } |
There was a problem hiding this comment.
Why is the PR for adding tests and docs modifying functionality?
There was a problem hiding this comment.
Good point.
Tests wouldn't pass without -Text-segment, -Trodata-segment and -Tldata-segment as it were not implemented previously
should i split them into 2 PR - one for tests/docs and one for implemetation ?
or you want to have a look at those implementations ?
There was a problem hiding this comment.
I think what @parth-07 is mentioning is the commit message says adding tests but now we are implementing support. The issue needs to be properly documented and also the commit message.
There was a problem hiding this comment.
I'll update the commit message. Thanks for clarifying.
…options - Add tests for --section-start, -Ttext, -Tdata, -Tbss, -Ttext-segment - Add -Trodata-segment and -Tldata-segment support and tests - Implement -Ttext-segment, -Trodata-segment, -Tldata-segment options - Update userguide with option descriptions Resolves qualcomm#1099 Signed-off-by: Sai Sanjay Chikne <saichikne180201@gmail.com>
|
@quic-seaswara @parth-07 |
Signed-off-by: Sai Sanjay Chikne <saichikne180201@gmail.com>
40cf58c to
ed54426
Compare
|
|
||
| --section-start=sectionname=org | ||
| Assigns the absolute address org to the named output section. | ||
| You may use this option multiple times to place multiple sections at specific addresses. |
There was a problem hiding this comment.
What happens if you specify the --section-start with the same section name multiple times ? Can we add that to the documentation ?
| You may use this option multiple times to place multiple sections at specific addresses. | ||
|
|
||
| -Ttext=org | ||
| Same as ``--section-start`` with ``.text`` as the section name. |
There was a problem hiding this comment.
What if there is no .text section ?
| Same as ``--section-start`` with ``.text`` as the section name. | ||
|
|
||
| -Tdata=org | ||
| Same as ``--section-start`` with ``.data`` as the section name. |
There was a problem hiding this comment.
Same as previous, what if there is no data section ?
| if (llvm::opt::Arg *arg = Args.getLastArg(T::Trodata_segment)) { | ||
| uint64_t addr = 0; | ||
| if (!llvm::StringRef(arg->getValue()).getAsInteger(0, addr)) | ||
| Config.options().setRodataSegment(addr); |
There was a problem hiding this comment.
Is this handled in program layout ?
There was a problem hiding this comment.
It is currently parsed and stored but not yet handled in program layout. Would you like me to implement it in this PR or open a separate issue for it?
| if (llvm::opt::Arg *arg = Args.getLastArg(T::Tldata_segment)) { | ||
| uint64_t addr = 0; | ||
| if (!llvm::StringRef(arg->getValue()).getAsInteger(0, addr)) | ||
| Config.options().setLdataSegment(addr); |
There was a problem hiding this comment.
Is this handled in program layout ?
There was a problem hiding this comment.
It is currently parsed and stored but not yet handled in program layout. Would you like me to implement it in this PR or open a separate issue for it?
| if (llvm::opt::Arg *arg = Args.getLastArg(T::Ttext_segment)) { | ||
| uint64_t addr = 0; | ||
| if (!llvm::StringRef(arg->getValue()).getAsInteger(0, addr)) | ||
| Config.options().setTextSegment(addr); |
There was a problem hiding this comment.
It might be useful to give a warning with linker scripts if the user used a linker script and users using this option.
| if (llvm::opt::Arg *arg = Args.getLastArg(T::Ttext_segment)) { | ||
| uint64_t addr = 0; | ||
| if (!llvm::StringRef(arg->getValue()).getAsInteger(0, addr)) | ||
| Config.options().setTextSegment(addr); |
There was a problem hiding this comment.
Also add a diagnostic if the value is not represented as a integer or does not fit the target address space.
For example using a 64bit address on a 32bit image.
| Group<grp_scriptopts>; | ||
| defm Trodata_segment | ||
| : dashEqWithOpt<"Trodata-segment", "Trodata-segment", "Trodata_segment", | ||
| "Specify an address for the .rodata-segment segment">, |
There was a problem hiding this comment.
Can you reword this ? Is there anything like .rodata-segment ?
| Group<grp_scriptopts>; | ||
| defm Tldata_segment | ||
| : dashEqWithOpt<"Tldata-segment", "Tldata-segment", "Tldata_segment", | ||
| "Specify an address for the .ldata-segment segment">, |
There was a problem hiding this comment.
What is .ldata-segment ? Can you simplify this and reword ?
|
|
||
| void setImageBase(uint64_t Value) { ImageBase = Value; } | ||
|
|
||
| const std::optional<uint64_t> &textSegment() const { return TextSegment; } |
There was a problem hiding this comment.
Can we convert these functions to use verbs, get/set.
I recommend using getTextSegmentAddress(), it would be very clear.
Also update the text map file so that an user inspecting the text map file can know what is going on.
- Rename textSegment/rodataSegment/ldataSegment to getTextSegmentAddress/getRodataSegmentAddress/getLdataSegmentAddress - Fix -Trodata-segment and -Tldata-segment option descriptions - Update --section-start, -Ttext, -Tdata, -Tbss documentation Signed-off-by: Sai Sanjay Chikne <saichikne180201@gmail.com>
parth-07
left a comment
There was a problem hiding this comment.
@sai18022001 Can you please move the tests to test/Common subdirectory?
Additionally, can you please update the PR description and the commit message to refect that this PR is adding the functionality instead of just adding test for the functionality.
| // -Ttext-segment=value | ||
| if (llvm::opt::Arg *arg = Args.getLastArg(T::Ttext_segment)) { | ||
| uint64_t addr = 0; | ||
| if (!llvm::StringRef(arg->getValue()).getAsInteger(0, addr)) |
There was a problem hiding this comment.
We should report an error if the argument is incorrect for the new options added.
| } | ||
|
|
||
| uint64_t GNULDBackend::getImageBase(bool HasInterp, bool LoadEHdr) const { | ||
| if (auto TextSegment = config().options().getTextSegmentAddress()) |
There was a problem hiding this comment.
Can you please check what is the precedence between --image-base and -Ttext-segment in GNU ld?
There was a problem hiding this comment.
Why is this test hexagon specific?
Fixes #1099
Add missing
SectionStart.testfor Hexagon targetAdd Common tests for
-Ttext,-Tdata,-TbssAdd Common test for
-Ttext-segmentAdd x86_64 tests for
-Trodata-segmentand-Tldata-segmentUpdate userguide
layout.rstwith descriptions for all optionsModifications to support this fix:
Add
-Trodata-segmentand-Tldata-segmentoption definitions and implementationImplement
-Ttext-segmentoption (required for tests to pass)Note: When both
-Ttext-segmentand a linker script address assignment arespecified, the linker script takes precedence. This matches
GNU ldbehaviorand has been confirmed by the maintainer.