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

BaseTools/build_rule.template: Set additional Rust module linker flags #1098

Conversation

makubacki
Copy link
Member

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?
  • 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.

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.

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

codecov-commenter commented Aug 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 1.35%. Comparing base (835d06b) to head (f23c1f8).

Additional details and impacted files
@@                Coverage Diff                 @@
##           release/202311    #1098      +/-   ##
==================================================
+ Coverage            1.06%    1.35%   +0.28%     
==================================================
  Files                1303     1303              
  Lines              332238   333838    +1600     
  Branches             5103     5103              
==================================================
+ Hits                 3547     4512     +965     
- Misses             328204   329243    +1039     
+ Partials              487       83     -404     
Flag Coverage Δ
MdeModulePkg 0.68% <ø> (ø)
MdePkg 5.40% <ø> (+2.12%) ⬆️
NetworkPkg 0.55% <ø> (ø)
PolicyServicePkg 30.41% <ø> (ø)

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 e8b5c47 into microsoft:release/202311 Aug 8, 2024
37 checks passed
@os-d os-d mentioned this pull request Aug 8, 2024
5 tasks
makubacki added a commit to makubacki/mu_basecore that referenced this pull request Aug 9, 2024
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 added a commit that referenced this pull request Aug 9, 2024
#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>
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.

5 participants