[PATCH] [MIPS] Raise highest supported EI_ABIVERSION value

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] [MIPS] Raise highest supported EI_ABIVERSION value

Mihailo Stojanovic
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(-)

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 */
--
2.7.4

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] [MIPS] Raise highest supported EI_ABIVERSION value

Joseph Myers
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.)

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] [MIPS] Raise highest supported EI_ABIVERSION value

Mihailo Stojanovic
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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] [MIPS] Raise highest supported EI_ABIVERSION value

Joseph Myers
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.

--
Joseph S. Myers
[hidden email]