Bug 30915 - ld testsuite: visibility incorrectly marked XFAIL on musl targets
Summary: ld testsuite: visibility incorrectly marked XFAIL on musl targets
Status: ASSIGNED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.41
: P2 normal
Target Milestone: ---
Assignee: Nick Clifton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-09-29 04:43 UTC by A. Wilcox
Modified: 2023-10-05 10:53 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2023-10-05 00:00:00
Project(s) to access:
ssh public key:


Attachments
Proposed patch (384 bytes, patch)
2023-10-05 10:53 UTC, Nick Clifton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description A. Wilcox 2023-09-29 04:43:54 UTC
On an x86_64-foxkit-linux-musl system:

Running /home/awilcox/Code/binutils-next/system/binutils/src/binutils-2.41/ld/testsuite/ld-vsb/vsb.exp ...
XPASS: visibility (protected)
XPASS: visibility (protected) (PIC main)
XPASS: visibility (protected_undef_def)
XPASS: visibility (protected_undef_def) (PIC main)


On a armv7-foxkit-linux-musl system:

XPASS: visibility (hidden_undef) (non PIC)
XPASS: visibility (hidden_undef) (non PIC, load offset)
XPASS: visibility (hidden_undef) (PIC main, non PIC so)
XPASS: visibility (protected_undef) (non PIC)
XPASS: visibility (protected_undef) (non PIC, load offset)
XPASS: visibility (protected_undef) (PIC main, non PIC so)


Digging in, the x86_64 failures are because of lines 408-413 of vsb.exp:


            if { [ string match $visibility "protected" ]
                 || [ string match $visibility "protected_undef_def" ] } {
                if [ string match $support_protected "no" ] {
                    setup_xfail $target_triplet 
                }
            }


The "$support_protected" variable is set on lines 119-123:


    if [ld_compile "$CC_FOR_TARGET -g -DPROTECTED_CHECK" $srcdir/$subdir/main.c $tmpdir/main.o] {
      if [ld_link $CC_FOR_TARGET $tmpdir/main "$tmpdir/main.o"] {
        catch "exec $tmpdir/main" support_protected
      }
    }


Where the test's main.c is, in entirety, when PROTECTED_CHECK is defined:


#include <features.h>
#include <stdio.h>

int
main (void)
{
#if defined (__GLIBC__) && (__GLIBC__ > 2 \
                            || (__GLIBC__ == 2 \
                                &&  __GLIBC_MINOR__ >= 2))
  puts ("yes");
#else
  puts ("no");
#endif
  return 0;
}


That is.. not a very helpful test for musl targets.  I don't know the best way forward for this.  There is no preprocessor macro test for musl targets, so I would suggest something akin to changing the first line from:


#if defined (__GLIBC__) && (__GLIBC__ > 2 \
                            || (__GLIBC__ == 2 \
                                &&  __GLIBC_MINOR__ >= 2))


to:


#if !defined (__GLIBC__) || (__GLIBC__ > 2 \
                             || (__GLIBC__ == 2 \
                                 &&  __GLIBC_MINOR__ >= 2))


This would mean the protected-check is assumed to work on all implementations that are not glibc <2.2.  I don't know if some of the more "odd" targets (like Bionic) might then fail.  I know that µClibc defines __GLIBC__ == 2 and __GLIBC_MINOR__ == 2, so this at least shouldn't break that target.


Similarly, for armv7, we have lines 104-111 testing whether non-PIC tests should XFAIL:


    if [istarget arm*-*-*eabi*] {
        set file [open $tmpdir/movw-detect.c w]
        puts $file "void foo(void) { __asm (\"movw r0, #0\"); }"
        close $file
        if [run_host_cmd_yesno "$CC_FOR_TARGET" "$CFLAGS_FOR_TARGET -c $tmpdir/movw-detect.c -o $tmpdir/movw-detect.o"] {
            set shared_needs_pic "yes"
        }
    }


With a rationale in a block comment spanning lines 99-103:


    # On targets that have MOVW the compiler will emit relocations which
    # the linker doesn't support when compiling -shared without -fpic.  The
    # test to find out whether we want to XFAIL the non-PIC tests requires
    # a compile - so we pre-calculate it here.  We also note that this can
    # only affect arm*-*-*eabi* targets as the old ABI doesn't support v7.


This seems correct but incomplete:


/home/awilcox/trees/binutils-next/system/binutils/src/binutils-2.41/ld/.libs/lt-ld-new: tmpdir/sh1np.o: relocation R_ARM_THM_MOVW_ABS_NC against `shlibvar1' can not be used when making a shared object; recompile with -fPIC
tmpdir/sh1np.o: in function `shlib_check':
/home/awilcox/trees/binutils-next/system/binutils/src/binutils-2.41/ld/testsuite/ld-vsb/sh1.c:170:(.text+0x11c): dangerous relocation: unsupported relocation
/home/awilcox/trees/binutils-next/system/binutils/src/binutils-2.41/ld/.libs/lt-ld-new: tmpdir/sh1np.o: in function `visibility_checkfunptr':
/home/awilcox/trees/binutils-next/system/binutils/src/binutils-2.41/ld/testsuite/ld-vsb/sh1.c:216:(.text+0x124): undefined reference to `visibility'
[ .. snip .. ]


The unsupported relocation does happen but is not a fatal error so the test finds the matching regex line ("undefined reference to `visibility'") and XPASSes.


Each arch may be its own bug, but I'm not entirely sure, so I decided to file one bug to not clutter the tracker.  If the 32-bit Arm bug should be split out to its own bug, I can do that.
Comment 1 A. Wilcox 2023-09-30 07:26:48 UTC
However, patching the PROTECTED_CHECK in this way exposes that $shared_needs_pic should be "yes" for the protected tests on any 32-bit musl system.  For instance, on i586-foxkit-linux-musl:

FAIL: visibility (protected) (non PIC)
FAIL: visibility (protected) (non PIC, load offset)
FAIL: visibility (protected) (PIC main, non PIC so)
FAIL: visibility (protected_undef_def) (non PIC)
FAIL: visibility (protected_undef_def) (non PIC, load offset)
FAIL: visibility (protected_undef_def) (PIC main, non PIC so)

Naively changing $shared_needs_pic globally, unfortunately, causes a lot of other failures because it works for non-PIC in all cases except protected and protected_undef_def.
Comment 2 Nick Clifton 2023-10-05 10:52:50 UTC
(In reply to A. Wilcox from comment #0)
 
> #if defined (__GLIBC__) && (__GLIBC__ > 2 \
>                             || (__GLIBC__ == 2 \
>                                 &&  __GLIBC_MINOR__ >= 2))
 

> That is.. not a very helpful test for musl targets.  I don't know the best
> way forward for this.  There is no preprocessor macro test for musl targets,

Still ?  I would have thought that they had realized the problems that this causes and changed their minds by now.


> so I would suggest something akin to changing the first line from:
> [...]

Given the other problems you found with making this change, I would suggest that it would be safer to patch vsb.exp instead.   I am uploading a possible fix, although you may need to to tweak it in order to get it to work.  (I do not actually have a muscl installation with which I can test it).



> Similarly, for armv7, we have lines 104-111 testing whether non-PIC tests
> should XFAIL:

[This would have been better filed as a separate PR, even though it refers to code in the same file.  But since you have filed it here...]

> 
> The unsupported relocation does happen but is not a fatal error so the test
> finds the matching regex line ("undefined reference to `visibility'") and
> XPASSes.

So if we make the warnings fatal then the test will work ?

So if you change:

	if [run_host_cmd_yesno "$CC_FOR_TARGET" "$CFLAGS_FOR_TARGET -c $tmpdir/movw-detect.c -o $tmpdir/movw-detect.o"] {

to:

	if [run_host_cmd_yesno "$CC_FOR_TARGET" "$CFLAGS_FOR_TARGET -Wl,--fatal-warnings -c $tmpdir/movw-detect.c -o $tmpdir/movw-detect.o"] {

then the test should now work ?

Cheers
  Nick
Comment 3 Nick Clifton 2023-10-05 10:53:39 UTC
Created attachment 15155 [details]
Proposed patch