Patchwork [MIPS] Raise highest supported EI_ABIVERSION value

login
register
mail settings
Submitter Mihailo Stojanovic
Date Aug. 9, 2019, 2:26 p.m.
Message ID <1565360776-31536-1-git-send-email-mihailo.stojanovic@rt-rk.com>
Download mbox | patch
Permalink /patch/34016/
State New
Headers show

Comments

Mihailo Stojanovic - Aug. 9, 2019, 2:26 p.m.
Hello everyone,

As suggested by Joseph here [1], this bumps the highest valid ABIVERSION
value for ABSOLUTE ABI, which was, I suppose, overlooked by Maciej in
[2].

Cheers,
Mihailo

[1] https://sourceware.org/ml/libc-alpha/2019-07/msg00548.html
[2] https://sourceware.org/ml/libc-alpha/2018-07/msg00131.html

    * sysdeps/unix/sysv/linux/mips/ldsodefs.h: Increment highest valid
      ABIVERSION value.
---
 sysdeps/unix/sysv/linux/mips/ldsodefs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Joseph Myers - Aug. 9, 2019, 2:29 p.m.
On Fri, 9 Aug 2019, Mihailo Stojanovic wrote:

> Hello everyone,
> 
> As suggested by Joseph here [1], this bumps the highest valid ABIVERSION
> value for ABSOLUTE ABI, which was, I suppose, overlooked by Maciej in
> [2].

Please see what I said there about either adding a testcase (that fails 
before and passes after the patch), or including an explanation in the 
proposed commit message of why this is hard enough to test in the 
testsuite that it's appropriate not to include a test.  (Stating "this 
fixes test X that is already in the testsuite" is also an option if true.)
Mihailo Stojanovic - Aug. 13, 2019, 9:22 a.m.
Hello,

I wanted to discuss this a bit more before proposing another
version of the patch.

Seeing as this is a trivial ABI version increment proposition,
I believe that including a testcase is superfluous (even more
so considering that the original issue [1] has a corresponding
testcase).

Furthermore, testing this patch requires static linker cooperation,
which means the undefined hidden and internal weak symbol
handling in static linker must be checked during glibc configuration.

If you think that the testcase is needed anyway, what I had in
mind was checking the static linker in the configure script, and
then enabling/disabling the test based on the result. The test
would just need to execute without "ABI version invalid" error
message.

Cheers,
Mihailo

[1] https://sourceware.org/ml/libc-alpha/2018-06/msg00509.html
Joseph Myers - Aug. 13, 2019, 3:35 p.m.
On Tue, 13 Aug 2019, Mihailo Stojanović wrote:

> Seeing as this is a trivial ABI version increment proposition,
> I believe that including a testcase is superfluous (even more
> so considering that the original issue [1] has a corresponding
> testcase).

A corresponding testcase that did not discover this bug.  (I should add: 
as a bug that was user-visible in a release, it should also be filed in 
Bugzilla; then, once fixed, the bug should be marked RESOLVED / FIXED with 
the target milestone set to the first mainline release that will have the 
fix, so that it then appears in the automatically-generated list of fixed 
bugs in the NEWS file for that release.)

When we discover a bug in some feature / previous bug fix, that was not 
shown up by the tests added with that feature / bug fix, that indicates 
missing test coverage, and so a new test should typically be added along 
with the fix.

> Furthermore, testing this patch requires static linker cooperation,
> which means the undefined hidden and internal weak symbol
> handling in static linker must be checked during glibc configuration.
> 
> If you think that the testcase is needed anyway, what I had in
> mind was checking the static linker in the configure script, and
> then enabling/disabling the test based on the result. The test
> would just need to execute without "ABI version invalid" error
> message.

Yes, a test with such configure test support seems appropriate, if the 
test would fail when using an older static linker.  Please make sure a 
comment on the configure test says what binutils release was the first one 
with the fix, so that it's obvious at what point we can remove the 
configure test as no longer needed.

What exactly would go wrong when using an older static linker?  If the 
test would fail to link, then a configure test is needed.  If the test 
would simply wrongly PASS even without the rest of this glibc patch, 
because the older linker doesn't set EI_ABIVERSION for this, I don't think 
the configure test is needed.  I'm guessing this test does not need to 
check the values of symbols at runtime because the existing tests deal 
with that.

Patch

diff --git a/sysdeps/unix/sysv/linux/mips/ldsodefs.h b/sysdeps/unix/sysv/linux/mips/ldsodefs.h
index 8dde855..28257f8 100644
--- a/sysdeps/unix/sysv/linux/mips/ldsodefs.h
+++ b/sysdeps/unix/sysv/linux/mips/ldsodefs.h
@@ -34,7 +34,7 @@  extern void _dl_static_init (struct link_map *map);
 #undef VALID_ELF_ABIVERSION
 #define VALID_ELF_ABIVERSION(osabi,ver)			\
   (ver == 0						\
-   || (osabi == ELFOSABI_SYSV && ver < 4)		\
+   || (osabi == ELFOSABI_SYSV && ver < 5)		\
    || (osabi == ELFOSABI_GNU && ver < LIBC_ABI_MAX))
 
 #endif /* ldsodefs.h */