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

Compiles advice injectors down to Emit #1581

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Nov 20, 2024

Closes #1457

Left to do:

  • Rename AdviceInjector to NativeEvent (or NativeAdviceEvent?)

@plafer plafer force-pushed the plafer-1457-advice-injectors-emit branch from 0e738df to 91d8b85 Compare November 20, 2024 19:17
@plafer plafer requested a review from bobbinth November 20, 2024 19:17
@plafer plafer force-pushed the plafer-1457-advice-injectors-emit branch from 91d8b85 to 6487a01 Compare November 20, 2024 19:21
processor/src/lib.rs Outdated Show resolved Hide resolved
processor/src/lib.rs Outdated Show resolved Hide resolved
@plafer plafer force-pushed the plafer-1457-advice-injectors-emit branch from 6487a01 to 8cab631 Compare November 21, 2024 12:03
@plafer plafer force-pushed the plafer-1457-refactor-host-advice-provider branch from cb51a16 to f40dffc Compare November 21, 2024 12:12
Also renames the variant to `FalconSigToStack`
…eToStack`

In preparation for converting advice injectors to instructions. This parameter was never used, and is not crucial - the caller can always put the key on top of the stack before calling this injector.
@plafer plafer force-pushed the plafer-1457-advice-injectors-emit branch 2 times, most recently from b765077 to de02088 Compare November 21, 2024 12:32
@plafer plafer marked this pull request as ready for review November 21, 2024 13:53
@plafer plafer requested a review from bobbinth November 21, 2024 13:55
@plafer
Copy link
Contributor Author

plafer commented Nov 21, 2024

Last thing we could do in this PR is rename AdviceInjector to NativeEvent or something like that

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left some comments inline. Most are pretty small and the bigger once can be done in future PRs.

Once this and #1572 are merged (I would probably first merge this into #1572 and then merge #1572), we can close #1457. But let's also create an issue describing the remaining work on this (e.g., getting rid of AdviceInjector enum, moving some events to the standard library, coming up with a mechanism for handling events in an extensible manner etc.).

core/src/operations/decorators/mod.rs Show resolved Hide resolved
processor/src/operations/crypto_ops.rs Outdated Show resolved Hide resolved
processor/src/operations/crypto_ops.rs Outdated Show resolved Hide resolved
processor/src/operations/sys_ops.rs Outdated Show resolved Hide resolved
processor/src/operations/sys_ops.rs Outdated Show resolved Hide resolved
Comment on lines 136 to 133
if let Some(advice_injector) = AdviceInjector::from_event_id(event_id) {
self.handle_advice_injector(advice_injector, host)
} else {
host.on_event(self.into(), event_id)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR, but in the future we should probably split event_id into source and ID where source identifies a library (e.g., source=0 for the system events, source=1 for the standard library etc.). We've discussed some of this in #1457, but may be good to create a separate issue once #1457 is closed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My issue with this though is that it reduces the number of "non-colliding libraries" from potentially 2^32 down to 2^16. Also, most libraries will probably only define a few events, wasting ~2^16 bits of unused event IDs (but still allocated to them).

I figured it to be better to just have libraries subscribe to the set of event IDs they need, (which the host can track with a BTreeMap<u32, EventHandler>). Then, we're actually very unlikely for libraries to define the same event ID (assuming all libraries properly generate their event IDs randomly).

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's discuss this in a follow-up issue. For example, we could still have 32-bit space for libraries and then within each library we could have a 16-bit space for event IDs.

processor/src/operations/sys_ops.rs Outdated Show resolved Hide resolved
FalconSigToStack,
}

impl AdviceInjector {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not for this PR, but I'm wonder if we actually need the AdviceInjector enum at all. I think pretty much in all cases it can be replaced with Operation::Emit(EVENT_ID) and the code would be just as readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed AdviceInjector to SystemEvent, and I don't think it's so bad to have an enum. For example, in handle_system_event, we match on an enum instead of u32 constants, so if we add a SystemEvent, we'll get a compile error there (instead of nothing)

processor/src/host/advice/mod.rs Outdated Show resolved Hide resolved
@plafer plafer force-pushed the plafer-1457-advice-injectors-emit branch from 025ed0d to b82f2f1 Compare November 22, 2024 15:06
@plafer plafer merged commit 19cc0e7 into plafer-1457-refactor-host-advice-provider Nov 22, 2024
8 of 9 checks passed
@plafer plafer deleted the plafer-1457-advice-injectors-emit branch November 22, 2024 15:06
plafer added a commit that referenced this pull request Nov 22, 2024
…tors-emit

Compiles advice injectors down to `Emit`
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.

2 participants