-
Notifications
You must be signed in to change notification settings - Fork 435
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
rust: generate target specification files on the fly #694
Conversation
Now that we have host programs support, add a "Rust script" to generate the `rustc` target spec file. The script has been written so that its output matches exactly the current `arch/*/rust/*.json` static target files we have at the moment, to have a baseline and reduce the changes done by this commit. Thus the static files are now gone. The build system is set up in a way that we do not need to rebuild any Rust code as long as the `target.json` contents do not change, even if the kernel configuration changes for something else. Furthermore, the script does not need to be rebuilt even if the kernel configuration changes. With this, `KBUILD_RUST_TARGET` is also gone -- one should modify the script instead of trying to override the target file. After this, the script should be cleaned (e.g. remove unneeded keys) and fixed to configure the architectures properly (e.g. enable CPU features as needed, etc.). There were several design choices: - The language: this could have been done in a shell script, C or Perl. But as soon as Rust is an option, it is more convenient than any of those. And it is an ideal showcase for the Rust host programs support. Python could have also been a natural option for the script, though it cannot be used as part as the kernel compilation because it is not required (as far as I can see in the docs). Even if it were, Rust offers some advantages anyway (though the `json` module would have been fairly useful here). Compiling the Rust host program takes a lot of time compared to other languages, at least currently (e.g. 300ms), but there is no need to recompile it on config changes, so it is fine. And, of course, running it is very fast. - Whether or not to use conditional compilation. Initially I took advantage of the `rustc_cfg` flags, to make the script look closer to the actual Rust kernel code. However, that means the script depends on the config, which means it is not an independent tool anymore (though there is at least one other host program that gets passed the flags, but it is not common), plus requires recompiling it every time the configuration changes just to generate the actual file. It also made things more tricky on the build system. - Splitting the logic into `arch/*` or not. In order to have arch maintainers work independently, we could move part of the logic into their trees. However, given how small the script is and that some logic is shared anyway, it looks a little bit odd. So I decided to avoid it. We can revisit this if needed. - Whether to use a proper JSON generator or not, whether to use a `HashMap`, whether to set up a type that knows about all the keys that can be generated (or maybe even import part of the `rustc` code to be in sync), etc. The script has been written to be as simple as possible while making it easy to change the actual business logic. We should reevaluate as it gets developed. Signed-off-by: Miguel Ojeda <[email protected]>
I will take a look tomorrow. |
Thanks a lot! No rush. |
match self { | ||
Value::Boolean(boolean) => write!(formatter, "{}", boolean), | ||
Value::Number(number) => write!(formatter, "{}", number), | ||
Value::String(string) => write!(formatter, "\"{}\"", string), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could try {:?}
. This will add quotes around the string and escape at least things like quotes. It doesn't exactly match json, but it may be a bit less likely to trip up a json parser than what you have right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I had to change this due to the "\u0001__gnu_mcount_nc"
value ({:?}
escapes backslashes).
"e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128", | ||
); | ||
ts.push("disable-redzone", true); | ||
ts.push("emit-debug-gdb-scripts", false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one should probably be moved out of the target match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but please see the commit message, i.e. the intention of this PR is to keep the targets exactly the same as they are now, so that this commit is focused only about introducing the script, and then clean things up. Same for the other bits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
ts.push("max-atomic-width", 128); | ||
ts.push("needs-plt", true); | ||
ts.push("pre-link-args", pre_link_args_64); | ||
ts.push("stack-probes", stack_probes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one too I think.
ts.push("disable-redzone", true); | ||
ts.push("emit-debug-gdb-scripts", false); | ||
ts.push("features", "+strict-align,+neon,+fp-armv8"); | ||
ts.push("frame-pointer", "always"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this one.
} | ||
ts.push("code-model", "medium"); | ||
ts.push("disable-redzone", true); | ||
ts.push("emit-debug-gdb-scripts", false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this one.
ts.push("env", "gnu"); | ||
ts.push("function-sections", false); | ||
ts.push("linker-is-gnu", true); | ||
ts.push("os", if cfg.has("ARM") { "linux" } else { "none" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this always be "none"?
"e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128", | ||
); | ||
ts.push("disable-redzone", true); | ||
ts.push("emit-debug-gdb-scripts", false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
@bjorn3 Thanks a lot for the review, as usual! |
Now that we have host programs support, add a "Rust script"
to generate the
rustc
target spec file.The script has been written so that its output matches exactly
the current
arch/*/rust/*.json
static target files we haveat the moment, to have a baseline and reduce the changes done
by this commit. Thus the static files are now gone.
The build system is set up in a way that we do not need to rebuild
any Rust code as long as the
target.json
contents do not change,even if the kernel configuration changes for something else.
Furthermore, the script does not need to be rebuilt even if
the kernel configuration changes.
With this,
KBUILD_RUST_TARGET
is also gone -- one should modifythe script instead of trying to override the target file.
After this, the script should be cleaned (e.g. remove unneeded
keys) and fixed to configure the architectures properly (e.g.
enable CPU features as needed, etc.).
There were several design choices:
The language: this could have been done in a shell script,
C or Perl. But as soon as Rust is an option, it is more
convenient than any of those. And it is an ideal showcase for
the Rust host programs support.
Python could have also been a natural option for the script,
though it cannot be used as part as the kernel compilation
because it is not required (as far as I can see in the docs).
Even if it were, Rust offers some advantages anyway (though
the
json
module would have been fairly useful here).Compiling the Rust host program takes a lot of time compared
to other languages, at least currently (e.g. 300ms), but there
is no need to recompile it on config changes, so it is fine.
And, of course, running it is very fast.
Whether or not to use conditional compilation.
Initially I took advantage of the
rustc_cfg
flags, to makethe script look closer to the actual Rust kernel code.
However, that means the script depends on the config, which
means it is not an independent tool anymore (though there is
at least one other host program that gets passed the flags,
but it is not common), plus requires recompiling it every
time the configuration changes just to generate the actual
file. It also made things more tricky on the build system.
Splitting the logic into
arch/*
or not.In order to have arch maintainers work independently, we could
move part of the logic into their trees.
However, given how small the script is and that some logic
is shared anyway, it looks a little bit odd. So I decided
to avoid it. We can revisit this if needed.
Whether to use a proper JSON generator or not, whether to
use a
HashMap
, whether to set up a type that knows aboutall the keys that can be generated (or maybe even import
part of the
rustc
code to be in sync), etc.The script has been written to be as simple as possible
while making it easy to change the actual business logic.
We should reevaluate as it gets developed.
Signed-off-by: Miguel Ojeda [email protected]