[multitop] Add better support for linker scripts (preprocessing) #25908
+165
−26
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pamaury
changed the title
jwnrt
approved these changes
Jan 16, 2025
There was a problem hiding this comment.
I've gone over the Bazel part a couple of times and it looks good. Nice to see the get_tool_for_action used. (aside, I realised that I missed this for the other part of our repo that uses the preprocessor here: rules/ujson.bzl#L40).
I think this is better than using Bazel substitution as discussed in the other PR.
LGTM, thanks
Signed-off-by: Amaury Pouly <[email protected]>
pamaury
force-pushed
the
multitop_linker_scripts2
branch
2 times, most recently
from
c24080a to
9683aeb
Compare
a-will
reviewed
Jan 16, 2025
pamaury
force-pushed
the
multitop_linker_scripts2
branch
from
9683aeb to
5ac9ac0
Compare
|
One thing to note about this PR which may be confusing: it gives two ways to include files: |
Signed-off-by: Amaury Pouly <[email protected]>
With this commit, linker script files are now preprocessed. The is handled similarly to `cc_library`: if `ld_library` is used with only files in `includes`, then it simply adds them to the compilation context. If `ld_library` is used with `script` set, then the file with be preprocessed with the compilation context constructed from its dependencies. This commit also adds a `defines` attribute to the `ld_library` so that defines can be set in bazel BUILD files. Signed-off-by: Amaury Pouly <[email protected]>
This avoid including by mistake the wrong file: because it is marked as incompatible, it will fail to compile. Signed-off-by: Amaury Pouly <[email protected]>
Each top has a different top linker file name. When dependencies switch to multitop, then need to INCLUDE the correct file. This commit adds a define OPENTITAN_TOP_MEMORY_LD which is set to the file name so that user can do INCLUDE OPENTITAN_TOP_MEMORY_LD Signed-off-by: Amaury Pouly <[email protected]>
The OPENTITAN_TOP_MEMORY_LD define is set in each top's linker library target. Signed-off-by: Amaury Pouly <[email protected]>
pamaury
force-pushed
the
multitop_linker_scripts2
branch
from
5ac9ac0 to
d903a15
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alternative approach to #25785 that preprocesses linker scripts instead of using bazel templates.
For now, this PR choses the "easy" way of keep each's top linker file name and set a defines so that users that include the correct file without knowing the name. An alternative approach would be to rename the file so it's the same for all tops. Note that even renaming would not render the preprocess useless because further multitop changes down the line cannot be handled by just renaming.
Annoyance: I couldn't make
cc_common.compileto work because it does not add anything to thecoutoutput so one can't get the output file... I had to resort to manually run the compiler, I don't know if there is an easier way to do that. It's probably semi-overkill because for the purpose of preprocessing linker script, it's very unlikely that the environment or system includes will play any role but at least it's the "textbook" way of doing it.