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

RustAdvancedLoggerDxe: Remove spinlocks, add function!(), add std feature. #363

Conversation

joschock
Copy link
Contributor

@joschock joschock commented Nov 17, 2023

Description

This PR implements 3 enhancements for the RustAdvancedLoggerDxe driver:

  • Removes spinlocks altogether. State that used to be shared (in particular, the current log level while generating a multi-part log message) is now on the stack.
  • Add function!() macro that returns the current function name as a static string.
  • Add std feture that maps debug!() and by extension debugln!() macros to std::println;
  • Impacts functionality?
  • Impacts security?
  • Breaking change?
  • Includes tests?
    • Existing unit tests updated to accommodate new design.
  • Includes documentation?
    • Existing docs updated to accommodate new design.

How This Was Tested

Unit tests pass, new features validated in both unit test environment (std feature active and mapping to std::print) and qemu UEFI boot (std feature not active, using AdvLogger protocol).

Integration Instructions

N/A - existing code should function as-is. New code that wants to take advantage of std can activate it for tests by specifying it as a feature for RustAdvLoggerDxe in dev-dependencies:

[dev-dependencies]
RustAdvancedLoggerDxe = {workspace=true, features=["std"]}

@github-actions github-actions bot added impact:testing Affects testing type:documentation Improvements or additions to documentation labels Nov 17, 2023
@makubacki makubacki self-requested a review November 17, 2023 23:59
@makubacki
Copy link
Member

Will review on Monday.

@makubacki makubacki requested a review from apop5 November 21, 2023 21:58
@makubacki
Copy link
Member

@joschock, I thought this PR was going for a rebase to preserve the commits. If this the case, can you please drop the merge commit that's present?

There's a stack cookie breaking change rippling through the repos (impacting build at the moment). This can go in after that's done.

@joschock joschock force-pushed the personal/joschock/remove_advlogger_spinlocks branch from 2acae51 to 3e94300 Compare November 22, 2023 04:37
@joschock
Copy link
Contributor Author

@joschock, I thought this PR was going for a rebase to preserve the commits. If this the case, can you please drop the merge commit that's present?

There's a stack cookie breaking change rippling through the repos (impacting build at the moment). This can go in after that's done.

Thanks - yes that's right, clicked the merge out of habit. Reverted.

@makubacki makubacki enabled auto-merge (rebase) November 22, 2023 18:20
@makubacki makubacki disabled auto-merge November 27, 2023 20:03
@joschock joschock force-pushed the personal/joschock/remove_advlogger_spinlocks branch from 3e94300 to 8cf24c1 Compare November 29, 2023 17:40
@makubacki makubacki enabled auto-merge (rebase) November 29, 2023 17:41
@makubacki makubacki merged commit c2e0410 into microsoft:release/202302 Nov 29, 2023
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:testing Affects testing type:documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants