Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ocamlopt generates invalid MASM for C stubs using reserved identifiers #8901

Open
dra27 opened this issue Aug 28, 2019 · 5 comments
Open

ocamlopt generates invalid MASM for C stubs using reserved identifiers #8901

dra27 opened this issue Aug 28, 2019 · 5 comments
Assignees
Labels

Comments

@dra27
Copy link
Member

dra27 commented Aug 28, 2019

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: NEAR

However, 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 test

becomes

	mov	rax, OFFSET __camlMASM$test

then 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.

@dra27
Copy link
Member Author

dra27 commented Aug 28, 2019

(PS I'll be very happy to be told that MASM has a simpler way of doing this - I've struggled to find examples of this happening elsewhere)

@github-actions github-actions bot added the Stale label Aug 31, 2020
@shindere
Copy link
Contributor

shindere commented Aug 31, 2020 via email

@github-actions github-actions bot removed the Stale label Sep 2, 2020
@dra27
Copy link
Member Author

dra27 commented Sep 7, 2020

I'd need to page this back in (indeed, I have most if not all of a fix for it somewhere), but IIRC the problem here is user-code generating these symbols

@github-actions github-actions bot added the Stale label Sep 10, 2021
@ocaml ocaml deleted a comment from github-actions bot Sep 10, 2021
@ocaml ocaml deleted a comment from github-actions bot Sep 10, 2021
@dra27 dra27 removed the Stale label Sep 10, 2021
@dra27
Copy link
Member Author

dra27 commented Sep 10, 2021

Still an issue - I just pushed the code I'd started to put together in 2019 to dra27/ocaml.

@github-actions github-actions bot added the Stale label Sep 12, 2022
@dra27
Copy link
Member Author

dra27 commented Sep 13, 2022

I think that the only thing needed for the branch referenced is to lookup and add the complete list of MASM reserved keywords to it

@ocaml ocaml deleted a comment from github-actions bot Sep 13, 2022
@dra27 dra27 removed the Stale label Sep 13, 2022
@dra27 dra27 added the bug label Jan 26, 2023
@dra27 dra27 self-assigned this Jan 26, 2023
shym pushed a commit to shym/ocaml that referenced this issue Jan 18, 2024
shym pushed a commit to shym/ocaml that referenced this issue Jan 18, 2024
shym pushed a commit to shym/ocaml that referenced this issue Jan 18, 2024
shym pushed a commit to shym/ocaml that referenced this issue Jan 18, 2024
shym pushed a commit to shym/ocaml that referenced this issue Jan 19, 2024
shym pushed a commit to shym/ocaml that referenced this issue Jan 19, 2024
shym pushed a commit to shym/ocaml that referenced this issue Jan 22, 2024
shym pushed a commit to shym/ocaml that referenced this issue Jan 30, 2024
shym pushed a commit to shym/ocaml that referenced this issue Jan 30, 2024
shym pushed a commit to shym/ocaml that referenced this issue Jan 31, 2024
shym pushed a commit to shym/ocaml that referenced this issue Feb 1, 2024
dra27 added a commit to dra27/ocaml that referenced this issue Feb 1, 2024
dra27 added a commit to dra27/ocaml that referenced this issue Feb 1, 2024
dra27 added a commit to dra27/ocaml that referenced this issue Feb 1, 2024
shym pushed a commit to shym/ocaml that referenced this issue Feb 12, 2024
shym pushed a commit to shym/ocaml that referenced this issue Feb 13, 2024
dra27 added a commit to dra27/ocaml that referenced this issue Feb 13, 2024
shym pushed a commit to shym/ocaml that referenced this issue Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants