| Message ID | 4F102494.9050807@gjlay.de |
|---|---|
| State | New |
| Headers | show |
On Jan 13, 2012, at 4:33 AM, Georg-Johann Lay wrote: > This test case is obviously written for 32-bit platforms, thus added > "dg-require-effective-target ilp32" to ensure that the pointer mess won't lead > to FAILs because of > > warning: cast to pointer from integer of different size > > and > > warning: cast from pointer to integer of different size > > Ok to apply? Ok. > * gcc.dg/lto/20091013-1_0.c: Add dg-require-effective-target ilp32.
Mike Stump <mikestump@comcast.net> writes: > On Jan 13, 2012, at 4:33 AM, Georg-Johann Lay wrote: >> This test case is obviously written for 32-bit platforms, thus added >> "dg-require-effective-target ilp32" to ensure that the pointer mess won't lead >> to FAILs because of >> >> warning: cast to pointer from integer of different size >> >> and >> >> warning: cast from pointer to integer of different size >> >> Ok to apply? > > Ok. > >> * gcc.dg/lto/20091013-1_0.c: Add dg-require-effective-target ilp32. I wonder if the fix is right, though: the testcase currenly passes for 64-bit multilibs, but won't run there any longer if the patch is applied. Rainer
On Jan 13, 2012, at 9:23 AM, Rainer Orth wrote: > Mike Stump <mikestump@comcast.net> writes: > >> On Jan 13, 2012, at 4:33 AM, Georg-Johann Lay wrote: >>> This test case is obviously written for 32-bit platforms, thus added >>> "dg-require-effective-target ilp32" to ensure that the pointer mess won't lead >>> to FAILs because of >>> >>> warning: cast to pointer from integer of different size >>> >>> and >>> >>> warning: cast from pointer to integer of different size >>> >>> Ok to apply? >> >> Ok. >> >>> * gcc.dg/lto/20091013-1_0.c: Add dg-require-effective-target ilp32. > > I wonder if the fix is right, though: the testcase currenly passes for > 64-bit multilibs, but won't run there any longer if the patch is > applied. I rely upon words like obviously written for 32-bit platforms a little too much apparently, sorry. I agree, let's find some other solution. I've starred at the code, and is was not clear to me why you thought it was obviously for 32-bit platforms. So, I'd propose the -Wno- option silence the warning... Seem reasonable to people, if so, Ok.
Mike Stump wrote: > On Jan 13, 2012, at 9:23 AM, Rainer Orth wrote: >> Mike Stump <mikestump@comcast.net> writes: >> >>> On Jan 13, 2012, at 4:33 AM, Georg-Johann Lay wrote: >>>> This test case is obviously written for 32-bit platforms, thus added >>>> "dg-require-effective-target ilp32" to ensure that the pointer mess won't lead >>>> to FAILs because of >>>> >>>> warning: cast to pointer from integer of different size >>>> >>>> and >>>> >>>> warning: cast from pointer to integer of different size >>>> >>>> Ok to apply? >>> Ok. >>> >>>> * gcc.dg/lto/20091013-1_0.c: Add dg-require-effective-target ilp32. >> I wonder if the fix is right, though: the testcase currenly passes for >> 64-bit multilibs, but won't run there any longer if the patch is >> applied. > > I rely upon words like obviously written for 32-bit platforms a little too much apparently, sorry. I agree, let's find some other solution. I've starred at the code, and is was not clear to me why you thought it was obviously for 32-bit platforms. > > So, I'd propose the -Wno- option silence the warning... Seem reasonable to people, if so, Ok. The ilp32 is the closes match: The source casts pointer to int, int to pointer, long to int, uses 32-bit initializers for int, assumes size_t is unsigned long any maybe others. Test cases like these are a permanent annoyance. There are hundreds of test cases that are not properly written or reviewed or gated by dg-require-foo. I wonder why such test cases go into the test suite in the first place if the come with such bunch of implications. Most of the test cases run on *all* targets that GCC claims to support. Johann
On Fri, Jan 13, 2012 at 07:40:59PM +0100, Georg-Johann Lay wrote: > The ilp32 is the closes match: > > The source casts pointer to int, int to pointer, long to int, uses 32-bit > initializers for int, assumes size_t is unsigned long any maybe others. No. The source is just fine for any target where sizeof (long) == sizeof (void *). So both ilp32 and lp64. > I wonder why such test cases go into the test suite in the first place if the > come with such bunch of implications. Because they test actual problems that were reported and usually fixed with the same commit, to avoid regressing in the future? Jakub
Jakub Jelinek wrote: > On Fri, Jan 13, 2012 at 07:40:59PM +0100, Georg-Johann Lay wrote: >> The ilp32 is the closes match: >> >> The source casts pointer to int, int to pointer, long to int, uses 32-bit >> initializers for int, assumes size_t is unsigned long any maybe others. > > No. The source is just fine for any target where sizeof (long) == sizeof (void *). > So both ilp32 and lp64. > >> I wonder why such test cases go into the test suite in the first place if the >> come with such bunch of implications. > > Because they test actual problems that were reported and usually fixed with > the same commit, to avoid regressing in the future? > > Jakub So it's fine to dump any code to the test suite no matter on what platform it might break or work? I.e. it is sufficient that it runs on at least one platform that complies to the C standard (for C test case)? Are there really no policies for test case that they behave reasonably? If, for example, there were hundreds of testcases in the test suites that break if sizeof(void) != 2 adding to the fun working with your platform: What would you think? Johann
On Fri, Jan 13, 2012 at 08:25:39PM +0100, Georg-Johann Lay wrote: > So it's fine to dump any code to the test suite no matter on what platform it > might break or work? It is wrong to knowingly commit testcases that break on such platforms, but you really shouldn't expect every committer to test all testsuite additions on all targets. Jakub
Jakub Jelinek wrote: > On Fri, Jan 13, 2012 at 08:25:39PM +0100, Georg-Johann Lay wrote: >> So it's fine to dump any code to the test suite no matter on what platform it >> might break or work? > > It is wrong to knowingly commit testcases that break on such platforms, > but you really shouldn't expect every committer to test all testsuite > additions on all targets. > > Jakub I really don't expect anyone to run test suites on any platform or target supported by GCC. Such an approach is completely useless because of the amount of time and system resources and I never proposed that. But it appears that many of the test that are exposed to all targets are just dropped at the testsuite without even looking at them with respect to platform requirements. The most prominent of these requirements is sizeof(int) >= 4 because the most PRs are reported for such platforms. Still I think that patches that are applied to test suite should experience some review. Anything else will just increase annoyance and frustration of developers that work for platforms that are not in the center of bolide hardware. For many test cases it's already sufficient to cut down constants so that they fit into 16 bits, and if actually 32-bit variable is needed GCC provides magic like __INT32_TYPE__. Johann
On Jan 13, 2012, at 11:25 AM, Georg-Johann Lay wrote: > So it's fine to dump any code to the test suite no matter on what platform it > might break or work? No. It isn't fine, but it does happen. Some people spend a lot of time, trying to get the testcases minimal, portable and correct. Other people, can't even be bothered to test the testcase to ensure to actually even tested the failure. :-( The best way forward IMHO, is to not just fix it, but to provide feedback to those that author such testcases, to see if you can sensitize people to some of the issues. Some issues, like, ints are 16 bits, you're going to loose some or most people on most of the time. Other issues, should be easier. Timely feedback I think is the key though, best would be automated testing that can provide feedback within the hour, failing that, nightly.
Rainer Orth schrieb: > Mike Stump <mikestump@comcast.net> writes: > > >>On Jan 13, 2012, at 4:33 AM, Georg-Johann Lay wrote: >> >>>This test case is obviously written for 32-bit platforms, thus added >>>"dg-require-effective-target ilp32" to ensure that the pointer mess won't lead >>>to FAILs because of >>> >>>warning: cast to pointer from integer of different size >>> >>>and >>> >>>warning: cast from pointer to integer of different size >>> >>>Ok to apply? >> >>Ok. >> >>> * gcc.dg/lto/20091013-1_0.c: Add dg-require-effective-target ilp32. > > > I wonder if the fix is right, though: the testcase currenly passes for > 64-bit multilibs, but won't run there any longer if the patch is > applied. > > Rainer What about int32plus instead of ilp32? That's still not exactly what the test case needs but closer than ilp32. Johann
On 01/13/2012 08:21 PM, Georg-Johann Lay wrote: > I really don't expect anyone to run test suites on any platform or target > supported by GCC. Such an approach is completely useless because of the amount > of time and system resources and I never proposed that. > > But it appears that many of the test that are exposed to all targets are just > dropped at the testsuite without even looking at them with respect to platform > requirements. > > The most prominent of these requirements is sizeof(int) >= 4 because the most > PRs are reported for such platforms. > > Still I think that patches that are applied to test suite should experience > some review. Anything else will just increase annoyance and frustration of > developers that work for platforms that are not in the center of bolide hardware. > > For many test cases it's already sufficient to cut down constants so that they > fit into 16 bits, and if actually 32-bit variable is needed GCC provides magic > ike __INT32_TYPE__. I suggest listing these common testcase pitfalls / best practices somewhere, like for example, on a gcc wiki page. It's then easier to point people at the issues.
Index: gcc.dg/lto/20091013-1_0.c =================================================================== --- gcc.dg/lto/20091013-1_0.c (revision 183150) +++ gcc.dg/lto/20091013-1_0.c (working copy) @@ -1,5 +1,6 @@ /* { dg-lto-do link } */ /* { dg-require-effective-target fpic } */ +/* { dg-require-effective-target ilp32 } */ /* { dg-lto-options {{-fPIC -r -nostdlib -flto} {-fPIC -r -nostdlib -O2 -flto}} } */ void * HeapAlloc(void*,unsigned int,unsigned long);