Check for symbols with default visibility in symbol-check

[zmodem]

to ensure that compiler-builtins doesn't expose any symbols with default visibility. See discussion on rust-lang/rust#151998

[zmodem]

Lots of red on the CI, but that means the patch is working :-)

It's flagging the outline atomics targeted by the Rust-side PR. Also __aeabi_idivmod etc. which I'm assuming are similarly defined as naked_asm functions. Also __rust_probestack.

RSQRT_TAB is a variable though, so I guess that will need another patch to hide.

[tgross35]

Looking through the failures, all the naked functions fail as expected. But it seems like static RSQRT_TAB is also exposed? Any idea what's happening there?

[tgross35]

Oh sorry, you mentioned that in a comment already and I missed it :) Is that something that could also be fixed in rust-lang/rust#151998?

[zmodem]

Yes, I've updated that PR.

[tgross35]

I recently added some tests for symcheck, would you be able to add one to crates/symbol-check/tests/all.rs as well? It should look a bit like

compiler-builtins/crates/symbol-check/tests/all.rs

Lines 65 to 76 in a7b1d8e

	#[test]
	fn test_core_symbols() {
	let t = TestTarget::from_env();
	let dir = tempdir().unwrap();
	let lib_out = dir.path().join("libfoo.rlib");
	t.rustc_build(&input_dir().join("core_symbols.rs"), &lib_out, |cmd| cmd);
	let assert = t.symcheck_exe().arg(&lib_out).assert();
	assert
	.failure()
	.stderr_contains("found 1 undefined symbols from core")
	.stderr_contains("from_utf8");
	}

.

[zmodem]

I recently added some tests for symcheck,

Hmm, those tests don't play well with my visibility check :-)

The main issue is that the test is building things which are not compiler-builtins, such as executables and rust libraries, and those will have plenty of visible symbols.

I can avoid some of it by ignoring executables in my check, but for example the good function in good_lib.rs will be visible, and I can't fix that with -Zdefault-visibility=hidden because that's not available with stable rustc.

(I tried adding #![compiler_builtins] to good_lib.rs, but rustc didn't like that either.)

[zmodem]

I added a --no-visibility flag which the tests that don't care about visibility checking can pass. Does that sound like a reasonable solution?

[tgross35]

Ah sorry yeah, the whole symcheck thing and its tests are unfortunately a bit rough around the edges. It should be possible to add a C/C++ file using __attribute__ ((visibility ("hidden"))) and build it (like test_exe_gnu_stack_section does) if that works.

Alternatively I think it should always be using nightly rustc in CI? Not worth chasing things down too much though if it's not easy to get working.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[zmodem]

Ah sorry yeah, the whole symcheck thing and its tests are unfortunately a bit rough around the edges. It should be possible to add a C/C++ file using __attribute__ ((visibility ("hidden"))) and build it (like test_exe_gnu_stack_section does) if that works.

The linker will still insert a couple of visible symbols in the executable (__bss_start, _edata, _end I think).

I think having a flag is a reasonable fix? The default use case is checking for visible symbols in compiler-builtins libraries, and for other files -- as produced by some of these tests -- we can pass --no-visibility.

With rust-lang/rust#151998 in nightly, the tests are all green :-)

[tgross35]

Ah sorry yeah, the whole symcheck thing and its tests are unfortunately a bit rough around the edges. It should be possible to add a C/C++ file using __attribute__ ((visibility ("hidden"))) and build it (like test_exe_gnu_stack_section does) if that works.

The linker will still insert a couple of visible symbols in the executable (__bss_start, _edata, _end I think).

I think having a flag is a reasonable fix? The default use case is checking for visible symbols in compiler-builtins libraries, and for other files -- as produced by some of these tests -- we can pass --no-visibility.

I was referring to a positive test that makes sure we don't flag if there are global symbols that aren't visible, e.g.

root@52460572143d:~# cat hidden-vis.c
__attribute__ ((visibility ("hidden"))) int bar() {}
root@52460572143d:~# gcc hidden-vis.c -c
root@52460572143d:~# readelf -s hidden-vis.o

Symbol table '.symtab' contains 11 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND
     1: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS hidden-vis.c
     2: 0000000000000000     0 SECTION LOCAL  DEFAULT    1 .text
     3: 0000000000000000     0 SECTION LOCAL  DEFAULT    2 .data
     4: 0000000000000000     0 SECTION LOCAL  DEFAULT    3 .bss
     5: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT    1 $x
     6: 0000000000000000     0 SECTION LOCAL  DEFAULT    5 .note.GNU-stack
     7: 0000000000000014     0 NOTYPE  LOCAL  DEFAULT    6 $d
     8: 0000000000000000     0 SECTION LOCAL  DEFAULT    6 .eh_frame
     9: 0000000000000000     0 SECTION LOCAL  DEFAULT    4 .comment
    10: 0000000000000000     8 FUNC    GLOBAL HIDDEN     1 bar

But I suppose that case is just covered by default when running tests on c-b, so LGTM.

Thank you for working on this!