Stream: git-cranelift

Topic: cranelift / Issue #1353 de-public `fn finalize_definitions`

GitHub (Jan 16 2020 at 23:28):

iximeow opened Issue #1353:

Now that finalize_definitions is called by default when finalizing a module (since #1290), calling it in an application using Cranelift is likely either an unintentional second call to finalize_definitions, or an outright error. If the lifecycle of finalization is entirely internal to Module (as it seems it ought to be), we ought to make this non-public and avoid some misuse.

I happened to trip across this when revisiting some code written before this change was made, where this might have more clearly signalled what changed in the mean time.

GitHub (Jan 17 2020 at 00:12):

philipc commented on Issue #1353:

This is true for cranelift-faerie and cranelift-object, but I don't think it is true for cranelift-simplejit. I'm not very familiar with cranelift-simplejit (who uses it?), but it seems to me that for it you need to call finalize_definitions, then run the code, then call finish when you are done (e.g. see rustc_codegen_cranelift).

So maybe a better fix is to revert #1290 and handle the cranelift-object relocations in finish instead of finalize_definitions.

Also, I thought you were using cranelift-faerie, and I thought it doesn't do anything to finalize anyway? Did I miss something?

GitHub (Jan 17 2020 at 01:33):

iximeow commented on Issue #1353:

I _think_ cranelift-simplejit is only used in a demo (there was an issue reported a little while ago related to said demo being bitrotted, even?) but generally if we can't guarantee finalize_definitions must be called before finish then I agree we probably should revert #1290 and adjust cranelift-object appropriately.

re. cranelift-faerie, I'm revisiting #902 for unwinding in lucet-runtime, which does add logic to publish. I think that would need to stick around even if much of the rest has been supplanted by the FDE work merged this week, but I'm still figuring that out.

GitHub (Jan 17 2020 at 01:59):

philipc commented on Issue #1353:

Could that logic you added in publish be moved to finish too?

GitHub (Jan 17 2020 at 02:20):

iximeow commented on Issue #1353:

I don't see a reason why not. Pretty sure I picked finalize_definitions because it seemed like writing .eh_frame is part of finalizing definitions in a module, but it fits with finish fine. I'd add that the doc comment on finalize_definitions might be better off referencing cranelift-simplejit for an example of what applicable logic for finalize_definitions is.

GitHub (Jan 17 2020 at 03:10):

jyn514 commented on Issue #1353:

I'm not very familiar with cranelift-simplejit (who uses it?), but it seems to me that for it you need to call finalize_definitions, then run the code, then call finish when you are done (e.g. see rustc_codegen_cranelift).

Since there's a valid use case for calling finalize_definitions before finish, is there a reason not to keep both functions as public?

GitHub (Jan 17 2020 at 03:20):

philipc commented on Issue #1353:

Since there's a valid use case for calling finalize_definitions before finish, is there a reason not to keep both functions as public?

Right, so what I'm proposing is keep them both public, and move cranelift-object's relocation processing to finish, and then #1290 is no longer necessary. (This is also what I proposed in https://github.com/bytecodealliance/cranelift/issues/1288#issuecomment-565753519).

GitHub (Jan 17 2020 at 03:24):

jyn514 commented on Issue #1353:

Ok, in that case I think there should be more documentation about what it means to 'finish' a module vs 'finalize_definitions'. I know it's meant to be independent of the backend, but you could give examples that cranelift-object and cranelift-faerie emits relocations after finish() and cranelift-simplejit compiles functions after finalize_definitions.

GitHub (Jan 20 2020 at 17:24):

bnjbvr commented on Issue #1353:

As a random thought, it would be good to rename both "finish" and "finalize_definitions" into some things that are more self-explanatory and help distinguishing between the twos, if we could.

GitHub (Jan 21 2020 at 03:52):

philipc commented on Issue #1353:

@iximeow I can work on fixing this if you haven't already.

GitHub (Jan 21 2020 at 16:59):

iximeow commented on Issue #1353:

@philipc go for it, i'm pretty sure i have my week cut out for me already

GitHub (Feb 10 2020 at 10:42):

bnjbvr closed Issue #1353:

Now that finalize_definitions is called by default when finalizing a module (since #1290), calling it in an application using Cranelift is likely either an unintentional second call to finalize_definitions, or an outright error. If the lifecycle of finalization is entirely internal to Module (as it seems it ought to be), we ought to make this non-public and avoid some misuse.

I happened to trip across this when revisiting some code written before this change was made, where this might have more clearly signalled what changed in the mean time.

Last updated: Nov 22 2024 at 17:03 UTC