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

[2405] BaseTools/build_rule.template: Set additional Rust module linker flags #1103

Conversation

makubacki
Copy link
Member

Description

Cherry-pick of 202311 change #1098 (e8b5c47)


This change sets the ImageBase in the PE header for Rust modules to 0 so they do not have a preferred base. This is similar to the EFI images produced by the edk2 build system. The subsystem type is also set to efi_boot_service_driver instead of the default target specification value of EFI_APPLICATION. Details for changing the subsystem type are here:

https://doc.rust-lang.org/nightly/rustc/platform-support/unknown-uefi.html#requirements

Ideally, these values would be set as individual
target.<triple>.rustflags in .cargo/config.toml. However, we override the /MAP argument using -C linker-args in build_rule.txt to the build output directory. This must be set dynamically since the output directory and module name are based on per module values.

Since the cargo configuration file does not support reading environment variables and setting an environment there in a [env] section would be too late to impact the commands that run in build_rules.txt (cargo is called from cargo make based on those rules), this is the simplest approach to retain the map file path in addition to the new changes.

In the future, this may be moved to a common target specification so the values are available without these changes.

  • Impacts functionality?
  • Impacts security?
  • Breaking change?
  • Includes tests?
  • Includes documentation?

How This Was Tested

  • Checked ImageBase and Subsystem of EFI images in output directory to confirm expected values.

Integration Instructions

This change is marked as breaking in case flows were dependent on the previous behavior. Otherwise, no changes are nedeed.

microsoft#1098)

## Description

This change sets the ImageBase in the PE header for Rust modules to `0`
so they do not have a preferred base. This is similar to the EFI images
produced by the edk2 build system. The subsystem type is also set to
`efi_boot_service_driver` instead of the default target specification
value of `EFI_APPLICATION`. Details for changing the subsystem type are
here:

https://doc.rust-lang.org/nightly/rustc/platform-support/unknown-uefi.html#requirements

Ideally, these values would be set as individual
`target.<triple>.rustflags` in `.cargo/config.toml`. However, we
override the `/MAP` argument using `-C linker-args` in `build_rule.txt`
to the build output directory. This must be set dynamically since the
output directory and module name are based on per module values.

Since the cargo configuration file does not support reading environment
variables and setting an environment there in a `[env]` section would be
too late to impact the commands that run in `build_rules.txt` (cargo is
called from cargo make based on those rules), this is the simplest
approach to retain the map file path in addition to the new changes.

In the future, this may be moved to a common target specification so the
values are available without these changes.

- [ ] Impacts functionality?
- [ ] Impacts security?
- [x] Breaking change?
- [ ] Includes tests?
- [ ] Includes documentation?

## How This Was Tested

- Checked `ImageBase` and `Subsystem` of EFI images in output directory
  to confirm expected values.

## Integration Instructions

This change is marked as breaking in case flows were dependent on the
previous behavior. Otherwise, no changes are nedeed.

Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
@makubacki makubacki self-assigned this Aug 9, 2024
@github-actions github-actions bot added impact:breaking-change Requires integration attention impact:non-functional Does not have a functional impact labels Aug 9, 2024
@makubacki makubacki enabled auto-merge (rebase) August 9, 2024 23:21
@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (release/202405@8c05410). Learn more about missing BASE report.

Additional details and impacted files
@@                Coverage Diff                @@
##             release/202405    #1103   +/-   ##
=================================================
  Coverage                  ?    1.55%           
=================================================
  Files                     ?     1447           
  Lines                     ?   360627           
  Branches                  ?     4197           
=================================================
  Hits                      ?     5622           
  Misses                    ?   354945           
  Partials                  ?       60           
Flag Coverage Δ
MdeModulePkg 0.61% <ø> (?)
MdePkg 5.41% <ø> (?)
NetworkPkg 0.55% <ø> (?)
PolicyServicePkg 30.41% <ø> (?)
UefiCpuPkg 4.77% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@makubacki makubacki merged commit 1906ad9 into microsoft:release/202405 Aug 9, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:breaking-change Requires integration attention impact:non-functional Does not have a functional impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants