RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Nick Clifton]

Hi Guys,

  PR 68913 notes that the test gcc.dg/lto/pr61886_0.c test fails on
  targets whose C library does not provide a __fread_chk function.

  Since the purpose of the test is to show that GCC will correctly
  discard the invocation of __fread_chk_warn, we do not need to actually
  link against a real __fread_chk function.  A dummy will do.

  Hence I would like to apply the patch below.  This patch resolves
  unexpected failures of the pr61886_0.c test on targets like spu-elf
  and sparc64-elf.

Cheers
  Nick

gcc/testsuite/ChangeLog
2015-12-18  Nick Clifton  <nickc@redhat.com>

	PR 68913
	* gcc.dg/lto/pr61886_0.c (__fread_chk): Provide a weak definition
	of this function.

Index: gcc/testsuite/gcc.dg/lto/pr61886_0.c
===================================================================
--- gcc/testsuite/gcc.dg/lto/pr61886_0.c	(revision 231805)
+++ gcc/testsuite/gcc.dg/lto/pr61886_0.c	(working copy)
@@ -4,7 +4,21 @@
 typedef __SIZE_TYPE__ size_t;
 typedef struct _IO_FILE FILE;
 
-extern size_t __fread_chk (void *__restrict __ptr, size_t __ptrlen, size_t __size, size_t __n, FILE *__restrict __stream) __asm__ ("" "__fread_chk")      __attribute__ ((__warn_unused_result__));
+/* PR 68913: Not all targets have __fread_chk available,
+   so we provide our own version for this function here.  */
+
+size_t __fread_chk (void *__restrict, size_t, size_t, size_t, FILE *__restrict)  __attribute__ ((__warn_unused_result__,weak));
+size_t
+__fread_chk (void *__restrict __ptr __attribute__((unused)),
+	     size_t __ptrlen __attribute__((unused)),
+	     size_t __size __attribute__((unused)),
+	     size_t __n __attribute__((unused)),
+	     FILE *__restrict __stream __attribute__((unused)))
+{
+  return 0;
+}
+
+
 extern size_t __fread_chk_warn (void *__restrict __ptr, size_t __ptrlen, size_t __size, size_t __n, FILE *__restrict __stream) __asm__ ("" "__fread_chk")      __attribute__ ((__warn_unused_result__)) __attribute__((__warning__ ("fread called with bigger size * nmemb than length " "of destination buffer")));
 
 extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__)) __attribute__ ((__artificial__)) __attribute__ ((__warn_unused_result__))

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Jeff Law]

On 12/18/2015 10:07 AM, Nick Clifton wrote:
> Hi Guys,
>
>    PR 68913 notes that the test gcc.dg/lto/pr61886_0.c test fails on
>    targets whose C library does not provide a __fread_chk function.
>
>    Since the purpose of the test is to show that GCC will correctly
>    discard the invocation of __fread_chk_warn, we do not need to actually
>    link against a real __fread_chk function.  A dummy will do.
>
>    Hence I would like to apply the patch below.  This patch resolves
>    unexpected failures of the pr61886_0.c test on targets like spu-elf
>    and sparc64-elf.
>
> Cheers
>    Nick
>
> gcc/testsuite/ChangeLog
> 2015-12-18  Nick Clifton  <nickc@redhat.com>
>
> 	PR 68913
> 	* gcc.dg/lto/pr61886_0.c (__fread_chk): Provide a weak definition
> 	of this function.
?  Isn'the purpose of this test to verify the function alias resolution 
code?  In which case, does having the weak definition send us down a 
different path in that code which would cause the original bug in 61886 
not to be tested?  ie, are we *sure* this does not compromise the test. 
  Given the painful history around 61886, I loathe the idea of losing 
coverage of that issue.

I guess I'd be a lot more comfortable with this change if we first 
verified  that with the fix for 61886 reverted and this patch applied 
that linux platforms will show the failures seen in 61886.  Given the 
number of changes for this BZ that show up in the comments, that may be 
a royal PITA to test.

Alternately, we can just limit this test to Linux targets.
jeff

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Nick Clifton]

Hi Jeff,

>>    PR 68913 notes that the test gcc.dg/lto/pr61886_0.c test fails on
>>    targets whose C library does not provide a __fread_chk function.
>>
>>    Since the purpose of the test is to show that GCC will correctly
>>    discard the invocation of __fread_chk_warn, we do not need to actually
>>    link against a real __fread_chk function.  A dummy will do.

> ?  Isn'the purpose of this test to verify the function alias resolution
> code?

I think that it is, but that the test does this by requiring that gcc 
only generates code that calls __fread_chk.  Ie it does not generate any 
code that calls __fread_chk_warn.  If gcc did generate code that calls 
__fread_chk_warn then that function's warning attribute would be 
triggered and we would get the warning message associated with it - and 
the test would fail.

Since the test only links, it does not execute, there is no need to have 
working versions of __fread_chk and __fread_chk_warn available.  All 
that is necessary is that a symbol called __fread_chk is available at 
link time.  (A symbol called __fread_chk_warn is not needed as 
referencing this symbol triggers a warning, which causes the test to 
fail).  So providing a weak definition of __fread_chk should be 
sufficient for those runtimes which do not provide their own definition.

At least that is my theory...

Cheers
   Nick

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Jeff Law]

On 12/22/2015 02:54 AM, Nick Clifton wrote:
> Hi Jeff,
>
>>>    PR 68913 notes that the test gcc.dg/lto/pr61886_0.c test fails on
>>>    targets whose C library does not provide a __fread_chk function.
>>>
>>>    Since the purpose of the test is to show that GCC will correctly
>>>    discard the invocation of __fread_chk_warn, we do not need to
>>> actually
>>>    link against a real __fread_chk function.  A dummy will do.
>
>> ?  Isn'the purpose of this test to verify the function alias resolution
>> code?
>
> I think that it is, but that the test does this by requiring that gcc
> only generates code that calls __fread_chk.  Ie it does not generate any
> code that calls __fread_chk_warn.  If gcc did generate code that calls
> __fread_chk_warn then that function's warning attribute would be
> triggered and we would get the warning message associated with it - and
> the test would fail.
But does the existence of the weak fread_chk change the interactions 
inside IPA & LTO in such a way as to compromise the test?

One way to find out would be to check out a compiler before Honza's 
change which fixed 61886, verify the test as written fails, verify the 
test with your new weak symbol also fails.  Honza didn't fix this until 
early December, so you don't have to go terribly far back.



>
> Since the test only links, it does not execute, there is no need to have
> working versions of __fread_chk and __fread_chk_warn available.  All
> that is necessary is that a symbol called __fread_chk is available at
> link time.  (A symbol called __fread_chk_warn is not needed as
> referencing this symbol triggers a warning, which causes the test to
> fail).  So providing a weak definition of __fread_chk should be
> sufficient for those runtimes which do not provide their own definition.
Right.  I'm not worried about any of this stuff.  I'm worried about the 
hideous problems with had in the IPA/LTO bits.  Just look at the long 
discussion in 61886.

jeff

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Nick Clifton]

Hi Jeff,

> But does the existence of the weak fread_chk change the interactions
> inside IPA & LTO in such a way as to compromise the test?
>
> One way to find out would be to check out a compiler before Honza's
> change which fixed 61886, verify the test as written fails, verify the
> test with your new weak symbol also fails.  Honza didn't fix this until
> early December, so you don't have to go terribly far back.

Darn you and your clever brain...  You were right.  My patch prevents 
the test from triggering the bug that was fixed by Honza's patch.

So I have come up with an alternative, simpler patch - define the 
__fread_chk symbol on the linker command line (overriding any definition 
that may or may not be present in the target's C library). Since this is 
a (final) link time redefinition rather than a compile time one, it does 
not interfere with the IPA/LTO code.

This patch works.  I tested it both with an x86_64-pc-linux-gnu 
toolchain and an arm-eabi toolchain both before Honza's patch was 
committed (both targets fail the test) and with today's gcc sources 
(both targets pass the test).

OK to apply ?

Cheers
   Nick

gcc/testsuite/ChangeLog
2016-01-07  Nick Clifton  <nickc@redhat.com>

	PR target/68913
	* gcc.dg/lto/pr61886_0.c: Use the linker's --defsym command line
	option to always provide a value for the __fread_chk symbol.

Index: gcc/testsuite/gcc.dg/lto/pr61886_0.c
===================================================================
--- gcc/testsuite/gcc.dg/lto/pr61886_0.c	(revision 232123)
+++ gcc/testsuite/gcc.dg/lto/pr61886_0.c	(working copy)
@@ -1,5 +1,5 @@
  /* { dg-lto-do link } */
-/* { dg-lto-options { { -flto -O2 -Werror } } } */
+/* { dg-lto-options { { -flto -O2 -Werror 
-Wl,--defsym,__fread_chk=0x1234 } } } */

  typedef __SIZE_TYPE__ size_t;
  typedef struct _IO_FILE FILE;
l

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Rainer Orth]

Hi Nick,

> Index: gcc/testsuite/gcc.dg/lto/pr61886_0.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/lto/pr61886_0.c	(revision 232123)
> +++ gcc/testsuite/gcc.dg/lto/pr61886_0.c	(working copy)
> @@ -1,5 +1,5 @@
>  /* { dg-lto-do link } */
> -/* { dg-lto-options { { -flto -O2 -Werror } } } */
> +/* { dg-lto-options { { -flto -O2 -Werror -Wl,--defsym,__fread_chk=0x1234
> } } } */

this assumes GNU ld and will break on all targets that use different
linkers.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Nick Clifton]

Hi Rainer.

>> +/* { dg-lto-options { { -flto -O2 -Werror -Wl,--defsym,__fread_chk=0x1234

> this assumes GNU ld and will break on all targets that use different
> linkers.

OK, how about this version instead ?

Cheers
   Nick

Index: gcc/testsuite/gcc.dg/lto/pr61886_0.c
===================================================================
--- gcc/testsuite/gcc.dg/lto/pr61886_0.c        (revision 232123)
+++ gcc/testsuite/gcc.dg/lto/pr61886_0.c        (working copy)
@@ -1,5 +1,6 @@
  /* { dg-lto-do link } */
  /* { dg-lto-options { { -flto -O2 -Werror } } } */
+/* { dg-extra-ld-options "-Wl,--defsym,__fread_chk=0x1234" { target { 
gld } } } */

  typedef __SIZE_TYPE__ size_t;
  typedef struct _IO_FILE FILE;

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Rainer Orth]

Hi Nick,

>>> +/* { dg-lto-options { { -flto -O2 -Werror -Wl,--defsym,__fread_chk=0x1234
>
>> this assumes GNU ld and will break on all targets that use different
>> linkers.
>
> OK, how about this version instead ?
>
> Cheers
>   Nick
>
> Index: gcc/testsuite/gcc.dg/lto/pr61886_0.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/lto/pr61886_0.c        (revision 232123)
> +++ gcc/testsuite/gcc.dg/lto/pr61886_0.c        (working copy)
> @@ -1,5 +1,6 @@
>  /* { dg-lto-do link } */
>  /* { dg-lto-options { { -flto -O2 -Werror } } } */
> +/* { dg-extra-ld-options "-Wl,--defsym,__fread_chk=0x1234" { target { gld
> } } } */
>
>  typedef __SIZE_TYPE__ size_t;
>  typedef struct _IO_FILE FILE;

No, we're back at square one then, i.e. the failure reported in PR
testsuite/68913:

output is:
Undefined                       first referenced
 symbol                             in file
__fread_chk                         c_lto_pr61886_0.o
ld: fatal: symbol referencing errors
collect2: error: ld returned 1 exit status

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Jeff Law]

On 01/07/2016 06:37 AM, Nick Clifton wrote:
> Hi Jeff,
>
>> But does the existence of the weak fread_chk change the interactions
>> inside IPA & LTO in such a way as to compromise the test?
>>
>> One way to find out would be to check out a compiler before Honza's
>> change which fixed 61886, verify the test as written fails, verify the
>> test with your new weak symbol also fails.  Honza didn't fix this until
>> early December, so you don't have to go terribly far back.
>
> Darn you and your clever brain...  You were right.  My patch prevents
> the test from triggering the bug that was fixed by Honza's patch.
I wouldn't say clever, just paranoid around this particular issue.
>
> So I have come up with an alternative, simpler patch - define the
> __fread_chk symbol on the linker command line (overriding any definition
> that may or may not be present in the target's C library). Since this is
> a (final) link time redefinition rather than a compile time one, it does
> not interfere with the IPA/LTO code.
>
> This patch works.  I tested it both with an x86_64-pc-linux-gnu
> toolchain and an arm-eabi toolchain both before Honza's patch was
> committed (both targets fail the test) and with today's gcc sources
> (both targets pass the test).
>
> OK to apply ?
As folks noted, I think this is still specific to the linker in use. 
Does it make sense to just limit this test to specific platforms, 
perhaps just x86/i686 linux for now.  68913 mentions ppc64-linux fails, 
it'd be nice to know why since I'd expect that has the _chk symbols.



jeff

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Andreas Schwab]

Jeff Law <law@redhat.com> writes:

> 68913 mentions ppc64-linux fails, it'd be nice to know why since I'd
> expect that has the _chk symbols.

It doesn't fail for me (and gcc-testresults has no hits).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Nick Clifton]

Hi Jeff,

> As folks noted, I think this is still specific to the linker in use.

True. :-(

I cannot think of any linker neutral way to add a link time symbol 
definition.  Any attempts to define the __fread_chk function via an 
object file will affects the ipa/lto code and prevent the test from working.

I did have one idea though - we could change the name of the function 
being tested from one that might not exist (__fread_chk) to one that 
definitely should exist (eg malloc).


> Does it make sense to just limit this test to specific platforms,
> perhaps just x86/i686 linux for now.

> 68913 mentions ppc64-linux fails,
> it'd be nice to know why since I'd expect that has the _chk symbols.

Actually 68913 references any Solaris target, not ppc64-linux.

We could restrict the test to any target that has the GNU linker.  That 
would probably cover most targets, and since the actual bug being 
checked by the testcase is a generic one, not a target specific one, we 
can be reasonably confident that if the problem resurfaces it will be 
detected.

So, two possible alternative versions of the patch are proposed below. 
Do either take your fancy ?

Cheers
   Nick

Index: gcc/testsuite/gcc.dg/lto/pr61886_0.c
===================================================================
--- gcc/testsuite/gcc.dg/lto/pr61886_0.c	(revision 232132)
+++ gcc/testsuite/gcc.dg/lto/pr61886_0.c	(working copy)
@@ -1,6 +1,11 @@
-/* { dg-lto-do link } */
+/* { dg-lto-do link { target gld } } */
  /* { dg-lto-options { { -flto -O2 -Werror } } } */
+/* { dg-extra-ld-options "-Wl,--defsym,__fread_chk=0x1234" { target { 
gld } } } */

+/* This test is currently restricted to targets that use the GNU linker.
+   For the reason why see PR68913 and the gcc-patches thread starting here:
+     https://gcc.gnu.org/ml/gcc-patches/2016-01/msg00212.html  */
+
  typedef __SIZE_TYPE__ size_t;
  typedef struct _IO_FILE FILE;


Or....

Index: gcc/testsuite/gcc.dg/lto/pr61886_0.c
===================================================================
--- gcc/testsuite/gcc.dg/lto/pr61886_0.c        (revision 232132)
+++ gcc/testsuite/gcc.dg/lto/pr61886_0.c        (working copy)
@@ -1,11 +1,10 @@
-/* { dg-lto-do link } */
+/* { dg-lto-do link { target gld } } */
  /* { dg-lto-options { { -flto -O2 -Werror } } } */
-
  typedef __SIZE_TYPE__ size_t;
  typedef struct _IO_FILE FILE;

-extern size_t __fread_chk (void *__restrict __ptr, size_t __ptrlen, 
size_t __size, size_t __n, FILE *__restrict __stream) __asm__ ("" 
"__fread_chk")      __attribute__ ((__warn_unused_result__));
-extern size_t __fread_chk_warn (void *__restrict __ptr, size_t 
__ptrlen, size_t __size, size_t __n, FILE *__restrict __stream) __asm__ 
("" "__fread_chk")      __attribute__ ((__warn_unused_result__)) 
__attribute__((__warning__ ("fread called with bigger size * nmemb than 
length " "of destination buffer")));
+extern size_t __malloc (void *__restrict __ptr, size_t __ptrlen, size_t 
__size, size_t __n, FILE *__restrict __stream) __asm__ ("") 
__attribute__ ((__warn_unused_result__));
+extern size_t __malloc_warn (void *__restrict __ptr, size_t __ptrlen, 
size_t __size, size_t __n, FILE *__restrict __stream) __asm__ ("") 
__attribute__ ((__warn_unused_result__)) __attribute__((__warning__ 
("fread called with bigger size * nmemb than length " "of destination 
buffer")));

  extern __inline __attribute__ ((__always_inline__)) __attribute__ 
((__gnu_inline__)) __attribute__ ((__artificial__)) __attribute__ 
((__warn_unused_result__))
  size_t
@@ -17,9 +16,9 @@
        if (!__builtin_constant_p (__size)
            || !__builtin_constant_p (__n)
            || (__size | __n) >= (((size_t) 1) << (8 * sizeof (size_t) / 
2)))
-        return __fread_chk (__ptr, __builtin_object_size (__ptr, 0), 
__size, __n, __stream);
+        return __malloc (__ptr, __builtin_object_size (__ptr, 0), 
__size, __n, __stream);
        if (__size * __n > __builtin_object_size (__ptr, 0))
-        return __fread_chk_warn (__ptr, __builtin_object_size (__ptr, 
0), __size, __n, __stream);
+        return __malloc_warn (__ptr, __builtin_object_size (__ptr, 0), 
__size, __n, __stream);
      }
  }

@@ -28,6 +27,6 @@
  int main ()
  {
    char file_contents[4096];

    char file_contents[4096];
-  /* We shouldn't get this resolved to a call to __fread_chk_warn.  */
+  /* We shouldn't get this resolved to a call to __malloc_warn.  */
    return fread (file_contents, 1, nmemb, fp);
  }

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Jakub Jelinek]

On Thu, Jan 07, 2016 at 05:31:13PM +0000, Nick Clifton wrote:
> -extern size_t __fread_chk (void *__restrict __ptr, size_t __ptrlen, size_t
> __size, size_t __n, FILE *__restrict __stream) __asm__ ("" "__fread_chk")
> __attribute__ ((__warn_unused_result__));
> -extern size_t __fread_chk_warn (void *__restrict __ptr, size_t __ptrlen,
> size_t __size, size_t __n, FILE *__restrict __stream) __asm__ (""
> "__fread_chk")      __attribute__ ((__warn_unused_result__))
> __attribute__((__warning__ ("fread called with bigger size * nmemb than
> length " "of destination buffer")));
> +extern size_t __malloc (void *__restrict __ptr, size_t __ptrlen, size_t
> __size, size_t __n, FILE *__restrict __stream) __asm__ ("") __attribute__
> ((__warn_unused_result__));
> +extern size_t __malloc_warn (void *__restrict __ptr, size_t __ptrlen,
> size_t __size, size_t __n, FILE *__restrict __stream) __asm__ ("")

The __asm__ ("") on both decls is very weird.  The original had
"" "__fread_chk" aka "__fread_chk" instead.
But malloc really has also different arguments...

	Jakub

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Nick Clifton]

Hi Jakub,


>> -extern size_t __fread_chk (void *__restrict __ptr, size_t __ptrlen, size_t
>> __size, size_t __n, FILE *__restrict __stream) __asm__ ("" "__fread_chk")
>> __attribute__ ((__warn_unused_result__));

>> +extern size_t __malloc (void *__restrict __ptr, size_t __ptrlen, size_t
>> __size, size_t __n, FILE *__restrict __stream) __asm__ ("") __attribute__
>> ((__warn_unused_result__));

> The __asm__ ("") on both decls is very weird.  The original had
> "" "__fread_chk" aka "__fread_chk" instead.

What does that do ?  I could not find a reference to it in the 
documentation.

> But malloc really has also different arguments...

True - thinking about it afterwards it occurred to me that renaming the 
function to just "fread" would be a lot better ...

Cheers
   Nick

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Nick Clifton]

Hi Guys,

   OK - how about this reformulation of the pr61886 test ?

   The patch changes references to __fread_chk with references to just 
fread, which I assume will be present in all target runtime libraries. 
I had to add some preprocessor trickery in order to ensure that 
__USER_LABEL_PREFIX__ is correctly prepended to the assembler name of 
the functions, but other than that the test remains the same.  No linker 
funny business this time.

   I have tested this patch with an x86_64 native (whose C runtime does 
include __fread_chk), an ARM cross compiler (using newlib, which does 
not provide __fread_chk) and with a V850 cross compiler (which uses 
newlib and which also defines __USER_LABEL_PREFIX to "_").  Prior to 
Honza's r231548 patch all three toolchains fail this patched test. 
Using today#s latest and greatest mainline gcc sources, all three 
toolchains pass the patched test.

   OK to apply ?

Cheers
   Nick

gcc/testsuite/ChangeLog
2016-01-08  Nick Clifton  <nickc@redhat.com>

	PR target/68913
	* gcc.dg/lto/pr61886_0.c: Rename the external function called
	to fread so that it will be found in all target runtimes.


Index: gcc/testsuite/gcc.dg/lto/pr61886_0.c
===================================================================
--- gcc/testsuite/gcc.dg/lto/pr61886_0.c	(revision 232157)
+++ gcc/testsuite/gcc.dg/lto/pr61886_0.c	(working copy)
@@ -4,12 +4,15 @@
  typedef __SIZE_TYPE__ size_t;
  typedef struct _IO_FILE FILE;

-extern size_t __fread_chk (void *__restrict __ptr, size_t __ptrlen, 
size_t __size, size_t __n, FILE *__restrict __stream) __asm__ ("" 
"__fread_chk")      __attribute__ ((__warn_unused_result__));
-extern size_t __fread_chk_warn (void *__restrict __ptr, size_t 
__ptrlen, size_t __size, size_t __n, FILE *__restrict __stream) __asm__ 
("" "__fread_chk")      __attribute__ ((__warn_unused_result__)) 
__attribute__((__warning__ ("fread called with bigger size * nmemb than 
length " "of destination buffer")));
+#define STRING1(a) #a
+#define STRING2(a) STRING1(a)

+extern size_t fread (void *__restrict __ptr, size_t __ptrlen, size_t 
__size, size_t __n, FILE *__restrict __stream) __asm__ 
(STRING2(__USER_LABEL_PREFIX__) "fread")      __attribute__ 
((__warn_unused_result__));
+extern size_t fread_warn (void *__restrict __ptr, size_t __ptrlen, 
size_t __size, size_t __n, FILE *__restrict __stream) __asm__ 
(STRING2(__USER_LABEL_PREFIX__) "fread")      __attribute__ 
((__warn_unused_result__)) __attribute__((__warning__ ("fread called 
with bigger size * nmemb than length " "of destination buffer")));
+
  extern __inline __attribute__ ((__always_inline__)) __attribute__ 
((__gnu_inline__)) __attribute__ ((__artificial__)) __attribute__ 
((__warn_unused_result__))
  size_t
-fread (void *__restrict __ptr, size_t __size, size_t __n,
+local_fread (void *__restrict __ptr, size_t __size, size_t __n,
         FILE *__restrict __stream)
  {
    if (__builtin_object_size (__ptr, 0) != (size_t) -1)
@@ -17,9 +20,9 @@
        if (!__builtin_constant_p (__size)
            || !__builtin_constant_p (__n)
            || (__size | __n) >= (((size_t) 1) << (8 * sizeof (size_t) / 
2)))
-        return __fread_chk (__ptr, __builtin_object_size (__ptr, 0), 
__size, __n, __stream);
+        return fread (__ptr, __builtin_object_size (__ptr, 0), __size, 
__n, __stream);
        if (__size * __n > __builtin_object_size (__ptr, 0))
-        return __fread_chk_warn (__ptr, __builtin_object_size (__ptr, 
0), __size, __n, __stream);
+        return fread_warn (__ptr, __builtin_object_size (__ptr, 0), 
__size, __n, __stream);
      }
  }

@@ -28,6 +31,6 @@
  int main ()
  {
    char file_contents[4096];
-  /* We shouldn't get this resolved to a call to __fread_chk_warn.  */
-  return fread (file_contents, 1, nmemb, fp);
+  /* We shouldn't get this resolved to a call to fread_warn.  */
+  return local_fread (file_contents, 1, nmemb, fp);
  }

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Jeff Law]

On 01/07/2016 10:31 AM, Nick Clifton wrote:
>
> I did have one idea though - we could change the name of the function
> being tested from one that might not exist (__fread_chk) to one that
> definitely should exist (eg malloc).
Not a bad idea.

>
>
>> Does it make sense to just limit this test to specific platforms,
>> perhaps just x86/i686 linux for now.
>
>> 68913 mentions ppc64-linux fails,
>> it'd be nice to know why since I'd expect that has the _chk symbols.
>
> Actually 68913 references any Solaris target, not ppc64-linux.
It was in one of the comments.  A. Schwab did some poking and can't find 
evidence of ppc64-linux failing though -- I suspect it was a user error 
of some sort.


Jeff

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Jeff Law]

On 01/08/2016 05:01 AM, Nick Clifton wrote:
> Hi Guys,
>
>    OK - how about this reformulation of the pr61886 test ?
>
>    The patch changes references to __fread_chk with references to just
> fread, which I assume will be present in all target runtime libraries. I
> had to add some preprocessor trickery in order to ensure that
> __USER_LABEL_PREFIX__ is correctly prepended to the assembler name of
> the functions, but other than that the test remains the same.  No linker
> funny business this time.
>
>    I have tested this patch with an x86_64 native (whose C runtime does
> include __fread_chk), an ARM cross compiler (using newlib, which does
> not provide __fread_chk) and with a V850 cross compiler (which uses
> newlib and which also defines __USER_LABEL_PREFIX to "_").  Prior to
> Honza's r231548 patch all three toolchains fail this patched test. Using
> today#s latest and greatest mainline gcc sources, all three toolchains
> pass the patched test.
>
>    OK to apply ?
>
> Cheers
>    Nick
>
> gcc/testsuite/ChangeLog
> 2016-01-08  Nick Clifton  <nickc@redhat.com>
>
>      PR target/68913
>      * gcc.dg/lto/pr61886_0.c: Rename the external function called
>      to fread so that it will be found in all target runtimes.
Ok.

Thanks for going the extra mile on this one.

jeff

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Dominique d'Humières]

> Le 7 janv. 2016 à 15:00, Nick Clifton <nickc@redhat.com> a écrit :
> 
> Hi Dominique,
> 
> On 07/01/16 13:42, Dominique d'Humières wrote:
>>> +/* { dg-lto-options { { -flto -O2 -Werror -Wl,--defsym,__fread_chk=0x1234 } } } */
>> 
>> This won’t work on darwin:
>> 
>> ld: unknown option: --defsym
>> collect2: error: ld returned 1 exit status
> 
> Oh bananas.
> 
> Does the pr61886 test work on Darwin without any changes ?  If it fails then is there a Darwin linker command line option to define a symbol ?

No:

Executing on host: /opt/gcc/build_w/gcc/xgcc -B/opt/gcc/build_w/gcc/ c_lto_pr61886_0.o  -m64     -fno-diagnostics-show-caret -fdiagnostics-color=never   -flto -O2 -Werror       -o gcc-dg-lto-pr61886-01.exe    (timeout = 300)
spawn -ignore SIGHUP /opt/gcc/build_w/gcc/xgcc -B/opt/gcc/build_w/gcc/ c_lto_pr61886_0.o -m64 -fno-diagnostics-show-caret -fdiagnostics-color=never -flto -O2 -Werror -o gcc-dg-lto-pr61886-01.exe
Undefined symbols for architecture x86_64:
  "__fread_chk", referenced from:
      _main in c_lto_pr61886_0.o
ld: symbol(s) not found for architecture x86_64
collect2: error: ld returned 1 exit status
compiler exited with status 1

Dominique

> This revised version of the patch works for me, but I do not have a Darwin native system on which I can test it.  Would you be able to try it out for me ?
> 
> Cheers
>  Nick
> 
> 
> Index: gcc/testsuite/gcc.dg/lto/pr61886_0.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/lto/pr61886_0.c        (revision 232123)
> +++ gcc/testsuite/gcc.dg/lto/pr61886_0.c        (working copy)
> @@ -1,5 +1,6 @@
> /* { dg-lto-do link } */
> /* { dg-lto-options { { -flto -O2 -Werror } } } */
> +/* { dg-extra-ld-options "-Wl,--defsym,__fread_chk=0x1234" { target { ! { *-*-darwin* } } } } */
> 
> typedef __SIZE_TYPE__ size_t;
> typedef struct _IO_FILE FILE;

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Nick Clifton]

Hi Dominique,

On 07/01/16 13:42, Dominique d'Humières wrote:
>> +/* { dg-lto-options { { -flto -O2 -Werror -Wl,--defsym,__fread_chk=0x1234 } } } */
>
> This won’t work on darwin:
>
> ld: unknown option: --defsym
> collect2: error: ld returned 1 exit status

Oh bananas.

Does the pr61886 test work on Darwin without any changes ?  If it fails 
then is there a Darwin linker command line option to define a symbol ?

This revised version of the patch works for me, but I do not have a 
Darwin native system on which I can test it.  Would you be able to try 
it out for me ?

Cheers
   Nick


Index: gcc/testsuite/gcc.dg/lto/pr61886_0.c
===================================================================
--- gcc/testsuite/gcc.dg/lto/pr61886_0.c        (revision 232123)
+++ gcc/testsuite/gcc.dg/lto/pr61886_0.c        (working copy)
@@ -1,5 +1,6 @@
  /* { dg-lto-do link } */
  /* { dg-lto-options { { -flto -O2 -Werror } } } */
+/* { dg-extra-ld-options "-Wl,--defsym,__fread_chk=0x1234" { target { ! 
{ *-*-darwin* } } } } */

  typedef __SIZE_TYPE__ size_t;
  typedef struct _IO_FILE FILE;

Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test

[Dominique d'Humières]

> +/* { dg-lto-options { { -flto -O2 -Werror -Wl,--defsym,__fread_chk=0x1234 } } } */

This won’t work on darwin:

ld: unknown option: --defsym
collect2: error: ld returned 1 exit status

TIA

Dominique