Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[multitop] Add better support for linker scripts (templating) #25785

Closed
wants to merge 5 commits into from

Conversation

pamaury
Copy link
Contributor

@pamaury pamaury commented Jan 7, 2025

Linker files are not preprocessed, which makes it hard to support several tops. In particular, including different files based on the top, or using a different region is very difficult. We currently have a semi-hack where we automatically include the directory of any include-ed file but it still requires that files have the same name which is not always the case.

The ld_library rule is modified to take a new substitution dictionary as input which is subject to location expansion. This allows for example to do in the linker script:

INCLUDE @@MYFILE@@

and then in the BUILD file:

ld_library(
  ...,
  substitutions = {
    "@@MYFILE@@": "$(location //path/to/my/file)",
  },
)

The rest of the commits add some substitutions for the ottf and test_rom, plus a final commit specifically for darjeeling.

Review commit by commit, skip the first which is a squashed version of #25580

Copy link
Contributor

@jwnrt jwnrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rule changes LGTM, thanks

@pamaury pamaury force-pushed the multitop_linker_scripts branch from 62c9259 to 233173e Compare January 7, 2025 12:19
@pamaury pamaury requested a review from Razer6 January 7, 2025 14:25
Copy link
Member

@Razer6 Razer6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me!

@pamaury pamaury force-pushed the multitop_linker_scripts branch from 233173e to 31e79ff Compare January 8, 2025 11:23
Linker files are not preprocessed, which makes it hard to support
several tops. In particular, including different files based on
the top, or using a different region is very difficult.

The `ld_library` rule is modified to take a new substitution
dictionary as input which is subject to location expansion. This
allows for example to do in the linker script:
```
INCLUDE @@myfile@@
```
and then in the BUILD file:
```
ld_library(
  ...,
  substitutions = {
    "@@myfile@@": "$(location //path/to/my/file)",
  },
)
```

Signed-off-by: Amaury Pouly <[email protected]>
@pamaury pamaury force-pushed the multitop_linker_scripts branch from 31e79ff to 6a3a2d6 Compare January 10, 2025 14:04
@Razer6 Razer6 added the CI:Rerun Rerun failed CI jobs label Jan 13, 2025
@github-actions github-actions bot removed the CI:Rerun Rerun failed CI jobs label Jan 13, 2025
Copy link
Contributor

@a-will a-will left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine, but I'll mention that the convention is to use the C preprocessor for this sort of thing. From a support perspective, bazel's APIs are more likely to change in an incompatible way than the C preprocessor, though the risk likely isn't high for this particular activity.

So for a cpp setup, required variables / definitions / macros would go in a required platform-specific header file, the includes paths specified on the command line would handle selecting the correct header, and you'd process it with the C preprocessor to generate the real / final linker script.

That said, for this simple substitution, you get basically the same thing with even the linker script language's own include command. It's not clear the templates in bazel do more than add another DSL to the list of things to know to work with the project (in addition to the required strings to substitute). /shrug

The critical part in this PR is more the abstraction of the path, which was hard-coded as earlgrey. But top-specific linker search paths, directories dedicated to specific targets, and INCLUDE directives would get the job done too (in the ordinary way embedded projects typically do it).

@pamaury
Copy link
Contributor Author

pamaury commented Jan 16, 2025

This seems fine, but I'll mention that the convention is to use the C preprocessor for this sort of thing. From a support perspective, bazel's APIs are more likely to change in an incompatible way than the C preprocessor, though the risk likely isn't high for this particular activity.

So for a cpp setup, required variables / definitions / macros would go in a required platform-specific header file, the includes paths specified on the command line would handle selecting the correct header, and you'd process it with the C preprocessor to generate the real / final linker script.

That said, for this simple substitution, you get basically the same thing with even the linker script language's own include command. It's not clear the templates in bazel do more than add another DSL to the list of things to know to work with the project (in addition to the required strings to substitute). /shrug

The critical part in this PR is more the abstraction of the path, which was hard-coded as earlgrey. But top-specific linker search paths, directories dedicated to specific targets, and INCLUDE directives would get the job done too (in the ordinary way embedded projects typically do it).

This makes sense, although this is now a bigger project ;) I have created #25908 to investigate this alternative, let's discuss there the details.

@pamaury pamaury changed the title [multitop] Add better support for linker scripts [multitop] Add better support for linker scripts (templating) Jan 16, 2025
@pamaury
Copy link
Contributor Author

pamaury commented Jan 16, 2025

Based on the feedback, I am closing in favor of #25908

@pamaury pamaury closed this Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants