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
joseph@codesourcery.com