Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test
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 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
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
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
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
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
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
Jeff Law 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, sch...@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
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
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
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
> +/* { 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
Re: RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test
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
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
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 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
RFA: PR 68913: Provide weak version of __fread_chk for PR61886 test
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
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__))
