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

5.1.0: ocaml is not LTO ready #12660

Open
kloczek opened this issue Oct 12, 2023 · 13 comments
Open

5.1.0: ocaml is not LTO ready #12660

kloczek opened this issue Oct 12, 2023 · 13 comments

Comments

@kloczek
Copy link

kloczek commented Oct 12, 2023

Looks like ocaml build fails when LTO is used. and buil fails

@kloczek
Copy link
Author

kloczek commented Oct 12, 2023

Sorry forgot attach output with errors
ocaml.FAIL.txt

@MisterDA
Copy link
Contributor

MisterDA commented Oct 13, 2023

Note: LTO stands for Link Time Optimization. Here's an excerpt of the errors generated:

runtime/prims.c:441:14: error: type of ‘caml_weak_set’ does not match original declaration [-Werror=lto-type-mismatch]
  441 | extern value caml_weak_set(void);
      |              ^
runtime/weak.c:229:16: note: type mismatch in parameter 1
  229 | CAMLprim value caml_weak_set (value ar, value n, value el)
      |                ^
runtime/weak.c:229:16: note: type ‘value’ should match type ‘void’
runtime/weak.c:229:16: note: ‘caml_weak_set’ was previously declared here

The error comes from how we're generating the list of primitives. In #11763 I tried to generate the list of primitives using accurate types, but we chose to default to a simple typedef value (*c_primitive)(void); for the primitives in prims.c, knowing that it was not correct to have the same function declared with different types across different compilation units, but probably not that bad. Turns out it makes the LTO build fail! (the simplified patches were merged in #11861).

If there are no other known blockers for building OCaml in LTO (e.g., something rendering it totally moot), it might be worthwhile to revisit the first version of the patches where we extracted the primitives with types in their original condition.

EDIT: or simply just disable the warning with -Wno-lto-type-mismatch if it's really just, well, a warning.

@kloczek
Copy link
Author

kloczek commented Oct 13, 2023

So this generator trashes LTO.

By default autoconf uses --enablable-warn-error so in such case build additionally fails.

@dra27
Copy link
Member

dra27 commented Oct 13, 2023

So this generator trashes LTO.

Can we quantify this a little better? If I configure tag 5.1.0 with ./configure CFLAGS='-flto=auto' --enable-ocamltest, I get a lot of -Wlto-type-mismatch warnings, but the build succeeds and the testsuite mostly passes (the warnings are leaking out into some of the bytecode builds). Is that warning actually preventing something from working with LTO?

By default autoconf uses --enablable-warn-error so in such case build additionally fails.

This is only true for development builds - releases build with --disable-warn-error by default.

@kloczek
Copy link
Author

kloczek commented Oct 13, 2023

Can we quantify this a little better?

As long as those warnings are present linker cannot fully optimise generated code.

@xavierleroy
Copy link
Contributor

As long as those warnings are present linker cannot fully optimise generated code.

This might not be a big loss, since these functions that are declared with the wrong type are put in a big table, and are called indirectly through this table. So, there will be no link-time inlining or optimization based on the calling context for these functions, I'm afraid.

However, the warning is annoying, and points to something not quite right in the bytecode interpreter of OCaml. #11763 is a step in the right direction, and I think we can do even more well-typed using an union type, but for that we need to use a better scripting language than sed...

@dra27
Copy link
Member

dra27 commented Oct 16, 2023

The warning doesn't seem to be emitted for the K&R declarations in 5.0, but presumably they must end up with the same LTO fate (big table, indirect calls) as "untyped" is presumably treated the same as "mis-typed"?

Fully typing the primitives I think is a much larger problem, though - we can sort out the OCaml runtime, but then we also have the stub-libraries to worry about for ocamlc -custom, et al, or we get the same warnings when compiling custom runtimes (e.g. for ocamltest - i.e. anything linking with the Unix library, etc.)

However, are we potentially accidentally over-thinking this? It only affects bytecode - the native runtime library links with correctly typed primitives, based on the external declatations in the OCaml code? So could we just consider silencing the LTO warning on the runtime as being the fix, possibly even specifically for prims.c (and equivalently in bytelink.ml?)?

@MisterDA
Copy link
Contributor

The warning is generated at link-time, so we'd need to pass -Wno-lto-type-mismatch to gcc when linking gcc ... -o runtime/ocamlrun runtime/prims.o runtime/libcamlrun.a .... The warning cannot be disabled by #pragma instructions in C source files.

@kloczek
Copy link
Author

kloczek commented Oct 16, 2023

The warning is generated at link-time, so we'd need to pass -Wno-lto-type-mismatch to gcc when linking gcc

You can exactly the same final effect (correctly linked all binaries) by use --disable-warn-error configure option.
In this case warnings are still visible so it is not possible to loose that something still needs to be done to clean that.

@dra27
Copy link
Member

dra27 commented Oct 16, 2023

@kloczek - a released version of OCaml does not have --enable-warn-error by default. Which branch are you building? If a released version of OCaml is building with -Werror that's a bug.

@dra27
Copy link
Member

dra27 commented Oct 16, 2023

@MisterDA - ah, my bad, it would just indeed to be in the appropriate OC_ flags, etc. I was sort of hoping we'd be able to limit the disabling of the warning to those symbols which we know are problematic (i.e. the primitives)

@kloczek
Copy link
Author

kloczek commented Oct 16, 2023

@kloczek - a released version of OCaml does not have --enable-warn-error by default. Which branch are you building? If a released version of OCaml is building with -Werror that's a bug.

Yep 👍 .. that what I've already wrote in #12660 (comment)

xavierleroy added a commit to xavierleroy/ocaml that referenced this issue Oct 28, 2023
"Accurate" means "with their proper types instead of the generic type
`value prim(void)`".

Fixes: ocaml#12660
xavierleroy added a commit to xavierleroy/ocaml that referenced this issue Oct 28, 2023
"Accurate" means "with their proper types instead of the generic type
`value prim(void)`".

Fixes: ocaml#12660
xavierleroy added a commit to xavierleroy/ocaml that referenced this issue Oct 31, 2023
"Accurate" means "with their proper types instead of the generic type
`value prim(void)`".

Partial fix for ocaml#12660
@xavierleroy
Copy link
Contributor

#12700 addresses a number of these "LTO type mismatch" warnings (which are quite useful even in a non-LTO context, actually!). Some warnings remain in conjunction with ocamlc -custom and perhaps in other contexts, to be investigated more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants