[BZ,#18970] : Reference of pthread_setcancelstate in libc.a

Message ID CAMe9rOrGYk6s94vvSVC1R7uBS-qGZaH8Pdwr62G+ZZdgnLKeig@mail.gmail.com
State Not applicable
Headers

Commit Message

H.J. Lu Sept. 17, 2015, 9:24 p.m. UTC
  On Thu, Sep 17, 2015 at 12:58 PM, Roland McGrath <roland@hack.frob.com> wrote:
> If this matters it should break some linknamespace test.  If there is no
> reference from anything involved in the ISO C implementation, then there is
> no actual problem.
>
> For example, wordexp is a POSIX.1 function, so it referring directly to the
> public names of other POSIX.1 functions is not a problem.
>
> A change like this is not necessarily worthless even if it is not
> necessary.  But you haven't said why you think the change is desireable.
> If it makes is easier to understand or maintain the code overall, then that
> could be sufficient reason.  But you have to make some sort of positive
> case for a change.

Is this a valid test?
  

Comments

Roland McGrath Sept. 17, 2015, 9:51 p.m. UTC | #1
> Is this a valid test?

No.  There should be no need for individual tests like that.  The
linknamespace tests should catch such things if the conform data is correct
(and if it's not, the right thing is to fix the conform data).

AFAIK fmtmsg is only specified in standards that put all POSIX.1 symbols
(including pthread_setcancelstate) in the implementation name space.
  
Joseph Myers Sept. 17, 2015, 9:57 p.m. UTC | #2
On Thu, 17 Sep 2015, Roland McGrath wrote:

> > Is this a valid test?
> 
> No.  There should be no need for individual tests like that.  The
> linknamespace tests should catch such things if the conform data is correct
> (and if it's not, the right thing is to fix the conform data).

See one of the caveats listed in linknamespace.pl:

# * Weak undefined symbols are ignored; however, if a code path that
# references one (even just to check if its address is 0) is executed,
# that may conflict with a definition of that symbol in the user's
# program.

I don't know what combination of false positives and real problems you'd 
find if linknamespace.pl stopped special-casing weak symbols.

> AFAIK fmtmsg is only specified in standards that put all POSIX.1 symbols
> (including pthread_setcancelstate) in the implementation name space.

fmtmsg is in XPG4, which doesn't have pthreads.
  
Roland McGrath Sept. 17, 2015, 10:03 p.m. UTC | #3
> On Thu, 17 Sep 2015, Roland McGrath wrote:
> 
> > > Is this a valid test?
> > 
> > No.  There should be no need for individual tests like that.  The
> > linknamespace tests should catch such things if the conform data is correct
> > (and if it's not, the right thing is to fix the conform data).
> 
> See one of the caveats listed in linknamespace.pl:
> 
> # * Weak undefined symbols are ignored; however, if a code path that
> # references one (even just to check if its address is 0) is executed,
> # that may conflict with a definition of that symbol in the user's
> # program.

Ah.  That is something to be concerned with.

That being the case, HJ's test is probably about right (but needs comments).

> I don't know what combination of false positives and real problems you'd 
> find if linknamespace.pl stopped special-casing weak symbols.

I think we should look into that.  The name space issues are orthogoal to
weakness in all scenarios I can think of.  If code reached by function foo
is affected at all by a symbol bar and bar is in the application name space
for some application that can use foo, then there is a problem.

> > AFAIK fmtmsg is only specified in standards that put all POSIX.1 symbols
> > (including pthread_setcancelstate) in the implementation name space.
> 
> fmtmsg is in XPG4, which doesn't have pthreads.

In that case the fix is actually necessary.  Thanks for pointing that out.


Thanks,
Roland
  
H.J. Lu Sept. 17, 2015, 10:29 p.m. UTC | #4
On Thu, Sep 17, 2015 at 2:51 PM, Roland McGrath <roland@hack.frob.com> wrote:
>> Is this a valid test?
>
> No.  There should be no need for individual tests like that.  The
> linknamespace tests should catch such things if the conform data is correct
> (and if it's not, the right thing is to fix the conform data).
>
> AFAIK fmtmsg is only specified in standards that put all POSIX.1 symbols
> (including pthread_setcancelstate) in the implementation name space.

Does it mean it is OK for all most of, if not all,  non-ANSI C functions,
in glibc to use all POSIX.1 symbols? This is the usage of any non-ANSI C
functions may pull in any POSIX.1 symbols.
  
H.J. Lu Sept. 17, 2015, 10:31 p.m. UTC | #5
On Thu, Sep 17, 2015 at 2:57 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Thu, 17 Sep 2015, Roland McGrath wrote:
>
>> > Is this a valid test?
>>
>> No.  There should be no need for individual tests like that.  The
>> linknamespace tests should catch such things if the conform data is correct
>> (and if it's not, the right thing is to fix the conform data).
>
> See one of the caveats listed in linknamespace.pl:
>
> # * Weak undefined symbols are ignored; however, if a code path that
> # references one (even just to check if its address is 0) is executed,
> # that may conflict with a definition of that symbol in the user's
> # program.
>
> I don't know what combination of false positives and real problems you'd
> find if linknamespace.pl stopped special-casing weak symbols.
>

I think it is a mistake to use weak symbols for this purpose.
  
Joseph Myers Sept. 17, 2015, 10:33 p.m. UTC | #6
On Thu, 17 Sep 2015, H.J. Lu wrote:

> On Thu, Sep 17, 2015 at 2:51 PM, Roland McGrath <roland@hack.frob.com> wrote:
> >> Is this a valid test?
> >
> > No.  There should be no need for individual tests like that.  The
> > linknamespace tests should catch such things if the conform data is correct
> > (and if it's not, the right thing is to fix the conform data).
> >
> > AFAIK fmtmsg is only specified in standards that put all POSIX.1 symbols
> > (including pthread_setcancelstate) in the implementation name space.
> 
> Does it mean it is OK for all most of, if not all,  non-ANSI C functions,
> in glibc to use all POSIX.1 symbols? This is the usage of any non-ANSI C
> functions may pull in any POSIX.1 symbols.

Although most supported feature test macros in glibc are supersets of 
POSIX.1:1990, it's also the case that ISO C functions use lots of 
non-ISO-C functions via their implementation-namespace __* names - if an 
ISO C function uses __foo (strong name to which weak foo is a POSIX.1 
alias) then __foo can't use bar (non-ISO-C function) although it can use 
__bar.
  
Roland McGrath Sept. 17, 2015, 10:33 p.m. UTC | #7
> Does it mean it is OK for all most of, if not all,  non-ANSI C functions,
> in glibc to use all POSIX.1 symbols? This is the usage of any non-ANSI C
> functions may pull in any POSIX.1 symbols.

In general I would have said that's true.  But as Joseph pointed out, there
are standards we support, such as XPG4, that do not include all of recent
vintages of POSIX.1.  So I'd say it's still true for things in POSIX.1-1990
(notably not including pthreads).  And sometimes that's enough for it to be
completely obvious: anything outside outside ISO C can use read, write, and
close.  But byeond that it quickly becomes so picayune that I think we are
better off looking at the conform/data files rather than trying to use any
easy "rules of thumb" like this.
  

Patch

diff --git a/stdlib/Makefile b/stdlib/Makefile
index 402466a..c99ad73 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -74,8 +74,8 @@  tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
 		   tst-makecontext3 bug-getcontext bug-fmtmsg1		    \
 		   tst-secure-getenv tst-strtod-overflow tst-strtod-round   \
 		   tst-tininess tst-strtod-underflow tst-tls-atexit	    \
-		   tst-setcontext3 tst-tls-atexit-nodelete
-tests-static	:= tst-secure-getenv
+		   tst-setcontext3 tst-tls-atexit-nodelete tst-fmtmsg-static
+tests-static	:= tst-secure-getenv tst-fmtmsg-static
 
 modules-names	= tst-tls-atexit-lib
 
diff --git a/stdlib/tst-fmtmsg-static.c b/stdlib/tst-fmtmsg-static.c
new file mode 100644
index 0000000..60e6642
--- /dev/null
+++ b/stdlib/tst-fmtmsg-static.c
@@ -0,0 +1 @@ 
+#include "tst-fmtmsg.c"
diff --git a/stdlib/tst-fmtmsg.c b/stdlib/tst-fmtmsg.c
index b7948c5..de8d16a 100644
--- a/stdlib/tst-fmtmsg.c
+++ b/stdlib/tst-fmtmsg.c
@@ -81,5 +81,11 @@  do_test (void)
   return result;
 }
 
+int
+pthread_setcancelstate (int *p)
+{
+  return p[0];
+}
+
 #define TEST_FUNCTION do_test ()
 #include "../test-skeleton.c"