-
Notifications
You must be signed in to change notification settings - Fork 1.2k
ocamlopt generates invalid MASM for C stubs using reserved identifiers #8901
Description
In #8897, @stedolan made the capital offence of naming a C stub function intended to test something test. Honestly. The assembler generated for:
external test : unit -> unit = "test"
test ()looks something like:
mov rax, OFFSET test
…
; External functions
…
EXTRN test: NEARHowever, MASM doesn't permit the use of processor instructions (amongst many other things) as symbol names so both lines with test will cause assembler errors, and not just any assembler errors - really, really weird ones. After much head bashing, I believe that the only way to accomplish this is to recognise that a reserved symbol is being used (i.e. when OFFSET is emitted) and decorate it. So:
mov rax, OFFSET testbecomes
mov rax, OFFSET __camlMASM$testthen when the External functions section is emitted, prefix it with:
OPTION NOKEYWORD:<test>
_camlMASM$test TEXTEQU <test>and then the later definition will be fine, since test is no longer a reserved word:
EXTRN test: NEAR(before you ask: OPTION NOKEYWORD has no reverse and it's not possible to scope it - it was added in order to allow assembler listings to continue using symbol names of CPU instructions added after the listing was written)
This can be solved by altering the signature of add_used_symbol in the amd64 and i386 emitters to return the symbol name to use; the function itself will then record the correct usage. It shouldn't be necessary to do anything with add_def_symbol because all ocamlopt-generated names are already valid MASM identifiers - this is only about dealing with non-ocamlopt generated object files. A few new MASM-specific constructs in X86_masm and Stephen may then name his functions as logically as he pleases.
It's a mildly noisy change to both the emitters, though, so before I make it I'd prefer the concept of the change to be approved. A proof of concept (creating a special-case renaming of test to _test for the OFFSET instruction only and inserting the OPTION NOKEYWORD construct for test) fixes the original test from #8897.