Load frametables of dynlink'd modules in batch#11935
Conversation
dra27
left a comment
There was a problem hiding this comment.
LGTM, modulo a possible code comment and a make promote is needed in the testsuite
asmcomp/asmlink.ml
Outdated
| 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
left a comment
There was a problem hiding this comment.
This looks good to me. (I cannot comment on the changes to asmlink.ml though, which look unrelated.)
|
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. |
(cherry picked from commit 33699c8)
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.