Load frametables of dynlink'd modules in batch #11935
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jan 25, 2023
dra27
reviewed
Jan 25, 2023
asmcomp/asmlink.ml
Outdated
| @@ -235,7 +235,9 @@ let make_startup_file ~ppf_dump units_list ~crc_interfaces = | |||
| List.flatten (List.map (fun (info,_,_) -> info.ui_defines) units_list) in | |||
| compile_phrase (Cmm_helpers.entry_point name_list); | |||
| let units = List.map (fun (info,_,_) -> info) units_list in | |||
| List.iter compile_phrase (Cmm_helpers.generic_functions false units); | |||
| List.iter compile_phrase | |||
| (Cmm_helpers.emit_preallocated_blocks [] | |||
There was a problem hiding this comment.
Although the testsuite will guard against this actually regressing, I think it's worth adding a comment as to why the call to emit_preallocated_blocks is made?
gadmm
approved these changes
Jan 26, 2023
|
The changes to asmlink are the bit that ensures all modules have a (possibly empty) |
|
I was unclear with my comment. Only for this bit I did not look deeper on the change because I am less familiar with this part of the compiler, but this still looks good to me, thanks to your helpful explanation in the PR description. |
gasche
pushed a commit
to gasche/ocaml
that referenced
this pull request
Feb 3, 2023
(cherry picked from commit 33699c8)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Each
Dynlinkedcmxsfile may contain many modules, all with their own frametables. Currently, each module is registered and initialised one at a time, rebuilding the frame table each time. This patch changes it to register all of the frame tables in bulk, and then initialise all of the modules.I also added some more error checking - currently, Dynlink silently ignores missing frametables, which seems bad. It turns out that the
gc_rootssymbol was always missing forshared_startupblocks (where it's not necessary), so I changedasmlinkto unconditionally include an emptygc_rootsblock in this case, as that seems cleaner than having this special case.