Skip to content

Silence ld warning with libasmrun_shared.so#13496

Merged
shindere merged 3 commits intoocaml:trunkfrom
dra27:ld-warning
Nov 5, 2024
Merged

Silence ld warning with libasmrun_shared.so#13496
shindere merged 3 commits intoocaml:trunkfrom
dra27:ld-warning

Conversation

@dra27
Copy link
Copy Markdown
Member

@dra27 dra27 commented Sep 29, 2024

The amd64 and power backends were missing .type and .size directives on caml_system.frametable which causes a warning from ld when linking against the shared runtime.

Prior to this PR, when linking with libasmrun.so, ld emits:

/usr/bin/ld: warning: type and size of dynamic symbol `caml_system.frametable' are not defined

I'd like to put this on 4.14, where the i386 backend also has the same issue.

Copy link
Copy Markdown
Contributor

@tmcgilchrist tmcgilchrist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The s390x runtime file is also missing the .size directive, it should be exactly the same as the one for power.S.

How would I go about testing this change?

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Sep 30, 2024

I thought that I fixed these by testing on precheck a couple of years ago, but that's definitely worth a look, thanks! The full test is:

$ cat hello.ml
print_endline "hello, world"
$ cat main.c
#define CAML_INTERNALS
#include <caml/callback.h>

int main_os(int argc, char_os **argv)
{
  caml_startup(argv);
  caml_shutdown();
  return 0;
}
$ ocamlopt -output-obj -o ocaml.o hello.ml
$ gcc -o hello ocaml.o main.c -I $(ocamlopt -where) -L $(ocamlopt -where) -lasmrun_shared -lm
/usr/bin/ld: warning: type and size of dynamic symbol `caml_system__frametable' are not defined

@tmcgilchrist
Copy link
Copy Markdown
Contributor

tmcgilchrist commented Sep 30, 2024 via email

@dra27 dra27 changed the title Silence ld warning with libasmrun.so Silence ld warning with libasmrun_shared.so Sep 30, 2024
@tmcgilchrist
Copy link
Copy Markdown
Contributor

It appears that the .type directive is enough to satisfy the linker on s390x and it will calculate the correct size when linking the final main. The only difference I could find when adding the .size directive is the section for caml_system.frametable is 0 without it and is 28 when I add it. The final sizes in the main executable are the same.

# With .type and no .size
tsmc@s390x-worker-01:~/ocaml$ nm -S --print-size runtime/s390x.o |grep frametable
0000000000000000 D caml_system.frametable

# With .type and .size
tsmc@s390x-worker-01:~/ocaml$ nm -S --print-size runtime/s390x.o |grep frametable
0000000000000000 0000000000000028 D caml_system.frametable

Interestingly if you just add .size and remove .type it also silences the warnings on s390x and amd64.

It feels more correct to have both but it seems to not make a difference. I leave it up to you @dra27

@NickBarnes
Copy link
Copy Markdown
Contributor

Given that we are readily able to emit the correct versions of both directives, I think we should. Apparently these warnings are relatively new; perhaps ld on s390x will become more stringent in future.

@shindere
Copy link
Copy Markdown
Contributor

shindere commented Oct 2, 2024 via email

runtime/power.S Outdated
.short -1 /* negative frame size => use callback link */
.short 0 /* no roots here */
.align 3
.size caml_system.frametable, .-caml_system.frametable
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with other platforms, you should probably add a ENDOBJECT macro and use it here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect my original thinking (in 2022) was along the lines that there was no alternate version so it didn't need a macro... how does 7be1ad0 (putting in both OBJECT and ENDOBJECT to both) look?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost good! The .section directives should be left outside of these macros, however.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (finally - sorry for the delay)

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Oct 3, 2024

I pushed an extra commit applying the pattern of 7be1ad0 (OBJECT..ENDOBJECT) so the s390x backend, but are we sure about this... we're emitting .size directives for the functions either??

@ghost
Copy link
Copy Markdown

ghost commented Oct 3, 2024

I pushed an extra commit applying the pattern of 7be1ad0 (OBJECT..ENDOBJECT) so the s390x backend, but are we sure about this... we're emitting .size directives for the functions either??

We should, eventually (not necessarily in this PR, though).

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Oct 21, 2024

I think one's good to go as steps in the correct direction? (and eliminating that pesky warning!)

@ghost
Copy link
Copy Markdown

ghost commented Oct 21, 2024

My approval (for what little it's worth) still stands in today's version.

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Oct 21, 2024

Your review & approval on these things is much appreciated, thank you!

dra27 added 3 commits October 23, 2024 17:51
amd64 and power were missing .type and .size directives on
caml_system.frametable which causes a warning from ld when linking
against the shared runtime.
@shindere
Copy link
Copy Markdown
Contributor

shindere commented Nov 5, 2024

Approving this on behalf of @dustanddreams

@shindere shindere merged commit 718a664 into ocaml:trunk Nov 5, 2024
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

Successfully merging this pull request may close these issues.

4 participants