mach: Restructure exception server to separate protected exception cases [crashpad/crashpad : main]
Justin Cohen (Gerrit)
Justin Cohen added 8 comments![]( "Open in Gerrit")
Mark MentovaiI don't love this. Is there a better way to do this?
Justin CohenI don't love this. Is there a better way to do this?
What do you think of this suggestion?
Done
Wrap the bodies of these cases in a compile-time if constexpr check
so they are only compiled and instantiated when Traits = MachExcTraits,
resolving compile-time type-safety issues.Justin CohenWhen the changes are made, adjust this accordingly.
Done
case MachExcTraits::kMachMessageIDExceptionRaiseIdentityProtected: {Justin CohenHere’s what I suggest:
1. Remove the `default:` label from this `switch` statement. See (5) below for the new way to handle that case.
2. After this `switch` statement, introduce _a new_ `switch` statement, but put it inside a single `if constexpr` (as on your line 490, modified by my comment there).
3. Move the handling of these two message types into that new `switch` statement.
4. This `case` line, and the one for `kMachMessageIDExceptionRaiseIdentityProtected`, will now be “protected” by the `if constexpr`, so you can rewrite `MachExcTraits` to the more natural and correct `Traits`.
5. Instead of ` default:` label in either `switch` statement, this function will change to put the code formerly in the `default:` label to be beyond both `switch` statements, outside of the `if constexpr`.It’s a small amount of additional hoisting, but I bet that the diff will remain essentially as clean as it is now, with primarily only whitespace indentation changes. The structure of the code will be cleaner and much more logical. The abuse of `MachExcTraits` values for `exc`-subsystem requests will come to an end.
The new structure may even enable further cleanups.
Done
if constexpr (!std::is_same_v<Traits, MachExcTraits>) {Justin CohenFrame this in the affirmative instead: you want to bail out here if `Traits` is `ExcTraits`.
Done
typename MachExcTraits::ExceptionRaiseIdentityProtectedRequest;Justin CohenThis can go back to being `Traits` now.
Done
MachExcTraits::MIGCheckRequestExceptionRaiseIdentityProtected(Justin CohenThis too.
Done
typename MachExcTraits::ExceptionRaiseIdentityProtectedReply;Justin CohenAnd this.
Done
if constexpr (!std::is_same_v<Traits, MachExcTraits>) {Justin CohenSame here, and also with the `MachExcTraits`/`Traits` changes that follow.
Done
Related details
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 3 comments![]( "Open in Gerrit")
Standard 32-bit `exc` does not support identity-protected exceptionsIt’s no longer the standard. How about:
“In contrast to to the modern `mach_exc`, the original 32-bit `exc` does not…”
?
This elegant restructuring ensures protected exception cases are onlyyes. elegant.
if constexpr (std::is_same_v<Traits, MachExcTraits>) {You still need `<type_traits>` for this. (You added it before, but now it got lost!)
Related details
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
This elegant restructuring ensures protected exception cases are onlyyes. elegant.
arg, I snipped that out and it snuck back in. patchset 3 was the correct one. This is what I get for letting an LLM write english.
Related details
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 2 comments![]( "Open in Gerrit")
into two distinct switch statements: 1. A switch statement for standard
exceptions compiled for all Traits. 2. A switch statement for
identity-protected exceptions nested inside
a single `if constexpr (std::is_same_v<Traits, MachExcTraits>)`
block.You undid the line-per-list-item organization for 1. and 2. (by accident?), but these two indented lines were left as though it’s still organized with a separate line per list item.
This elegant restructuring ensures protected exception cases are onlyJustin Cohenyes. elegant.
arg, I snipped that out and it snuck back in. patchset 3 was the correct one. This is what I get for letting an LLM write english.
arg, I snipped that out and it snuck back in. patchset 3 was the correct one. This is what I get for letting an LLM write english.
I recommend against that. Seeing what you think a change should do in English lets me review it to make sure that it actually does that.
If you (or a tool) write a change in code, then the LLM-generated commit message can only, at best, describe what your code actually does, which might not be what you intend for it to do. If you had a tool write your code for you, this is especially important.
Related details
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
1 comment
into two distinct switch statements: 1. A switch statement for standard
exceptions compiled for all Traits. 2. A switch statement for
identity-protected exceptions nested inside
a single `if constexpr (std::is_same_v<Traits, MachExcTraits>)`
block.You undid the line-per-list-item organization for 1. and 2. (by accident?), but these two indented lines were left as though it’s still organized with a separate line per list item.
You undid the line-per-list-item organization for 1. and 2. (by accident?), but these two indented lines were left as though it’s still organized with a separate line per list item.
(if it helps: sometimes having a leading space on the 1. or - helps your reformatter understand that it’s a list. Like:
```
- A switch statement for identity-protected exceptions nested inside
a single `if constexpr (std::is_same_v<Traits, MachExcTraits>)`
block.
```
instead of:
```
- A switch statement for identity-protected exceptions nested inside
a single `if constexpr (std::is_same_v<Traits, MachExcTraits>)` block.
Related details
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 2 comments![]( "Open in Gerrit")
Standard 32-bit `exc` does not support identity-protected exceptionsIt’s no longer the standard. How about:
“In contrast to to the modern `mach_exc`, the original 32-bit `exc` does not…”
?
Done
if constexpr (std::is_same_v<Traits, MachExcTraits>) {Justin CohenYou still need `<type_traits>` for this. (You added it before, but now it got lost!)
Done
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
crashpad-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
crashpa...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Unreviewed changes
8 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
mach: Restructure exception server to separate protected exception cases
In contrast to to the modern mach_exc, the original 32-bit exc does not
support identity-protected exceptions (e.g.,
EXCEPTION_IDENTITY_PROTECTED). Under the previous unified switch
statement design, the protected cases in ExcServer<Traits>::
MachMessageServerFunction were compiled (but dead) when Traits =
ExcTraits. This forced the use of raw pointer type casts to compile
mismatched exception code arrays (casting 64-bit exception codes to
32-bit exception codes), which was flagged during spanification and by
UBSan.
This change restructures the MachMessageServerFunction by splitting it
into two distinct switch statements:
- A switch statement for standard exceptions compiled for all Traits.
- A switch statement for identity-protected exceptions nested inside
a single `if constexpr (std::is_same_v<Traits, MachExcTraits>)` block.
This ensures protected exception cases are only compiled and
instantiated when Traits = MachExcTraits.
- M util/mach/exc_server_variants.cc
Code-Review: +1 by Mark Mentovai
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |