de-public fn finalize_definitions

[iximeow]

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.

[philipc]

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?

[iximeow]

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.

[philipc]

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

[iximeow]

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.

[jyn514]

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?

[philipc]

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 #1288 (comment)).

[jyn514]

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.

[bnjbvr]

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.

[philipc]

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

[iximeow]

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