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

Use named constants in the switcher #339

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

nwf
Copy link
Member

@nwf nwf commented Nov 7, 2024

Pull yet more out of #320. Thanks to @rmn30 for the suggestion to make these self-check a la assembly-helpers.h.

@nwf nwf requested review from davidchisnall and rmn30 November 7, 2024 19:26
@nwf nwf force-pushed the 202411-switcher-symbolize branch from af59dd8 to 4d21cc6 Compare November 7, 2024 23:10
* macro will report an error if the provided value does not equal the constexpr
* evaluation of `expression`.
*/
# define EXPORT_ASSEMBLY_NAME(name, val) \
Copy link
Collaborator

Choose a reason for hiding this comment

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

It took me a minute to work out why we need both of these macros. I guess this one is just the same as EXPORT_ASSEMBLY_EXPRESSION(name, name, val) but without a name collision? That makes me think maybe we should use {} around the definition below?

Copy link
Member Author

Choose a reason for hiding this comment

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

We might want the names available in C++, maybe, and the invoking source could, in principle, choose to put things in scope to hide them if desired, but because these are otherwise at top-level, just using {} doesn't do the trick, eliciting "expected unqualified-id" errors from the compiler.

@@ -75,6 +75,7 @@ namespace priv
constexpr size_t MCAUSE_LOAD_PAGE_FAULT = 13;
constexpr size_t MCAUSE_STORE_PAGE_FAULT = 15;
constexpr size_t MCAUSE_THREAD_EXIT = 24;
constexpr size_t MCAUSE_THREAD_INTERRUPT = 25;
constexpr size_t MCAUSE_CHERI = 28;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be a separate PR but these constants are needed in error handlers so we should make them more publicly available in the SDK.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a public heading (and this is why I don't like abbreviations in names of things: priv is short for 'privileged spec' not 'private').

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess there's nothing stopping users from include priv/riscv.h it just seems a bit weird. At any rate we should do this in relevant places in the examples/tests.

@nwf nwf force-pushed the 202411-switcher-symbolize branch 2 times, most recently from 1bdb9d3 to da330fe Compare November 8, 2024 15:31
@nwf nwf enabled auto-merge (rebase) November 8, 2024 15:51
@nwf nwf force-pushed the 202411-switcher-symbolize branch from da330fe to 8e7188c Compare November 8, 2024 15:52
nwf and others added 4 commits November 8, 2024 16:38
These are not really "trusted stack", and we're about to have some more.
While here, tweak a comment as per Robert's suggestion.

Co-authored-by: Robert Norton <robert.norton@microsoft.com>
Co-authored-by: Robert Norton <robert.norton@microsoft.com>
Co-authored-by: Robert Norton <robert.norton@microsoft.com>
@nwf nwf force-pushed the 202411-switcher-symbolize branch from 8e7188c to c661519 Compare November 8, 2024 16:44
@nwf nwf merged commit bccba2a into CHERIoT-Platform:main Nov 8, 2024
6 checks passed
@nwf nwf deleted the 202411-switcher-symbolize branch November 8, 2024 16:54
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.

3 participants