| Message ID | 87egejk7mj.fsf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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__))