[PATCH] D66887: [test-suite][WIP] Add GCC C Torture Suite as External Test Suite
Sam Elliott via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 19 10:18:22 PDT 2019
lenary marked 9 inline comments as done.
lenary added a comment.
Thanks for the review!
I've gone back through most of the tests to categorise why they fail more accurately. Update to the patch incoming.
This patch also adds more special cases to the build config - extra flags for clang, or the linker, or detection of specific functions or architectures. I wanted the config to be fairly simple to start with, but I see the value of ensuring we don't have to keep updating the config early on.
I have also decided to split this patch into two parts. This patch will hold only the configuration info (and not the tests themselves, which will be in a follow-up patch). This should make reviews easier, and I can ensure they are both committed together. I'm not sure the patch adding the tests themselves needs to be posted for review, given it is a verbatim import, but I would welcome feedback on that.
================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt:63-71
+ # Runtime failures
+ 20021127-1.c
+ 20031003-1.c
+ bitfld-3.c
+ bitfld-5.c
+ eeprof-1.c
+ noinit-attribute.c
----------------
kristof.beyls wrote:
> Thanks for adding comments as to why certain tests are disabled, I find that very useful!
> I wonder though whether "Runtime failures" are due to bugs in clang, or these test cases relying on non-standard behaviour. In other words, are you sure this is a problem with the test and not with clang?
>
> Maybe it could make sense to split the lists of tests to skip in 2: one list with tests being incorrect or dependent on gcc-only features; the other lists of tests that trigger bugs in clang? That way there's a clear indication of which tests ought to pass, and which tests are never expected to pass?
Thanks for the suggestion about a list of "naughty tests" vs a list of "clang could do better on these". I've factored that into the update, along with much better explanations of why each test is or isn't on the list.
================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt:91-93
+ # Must link with libm
+ 980709-1.c
+ float-floor.c
----------------
kristof.beyls wrote:
> I wonder what's stopping you from adding -lm to the link command line flags for these tests?
> I seem to remember that some other tests in the test-suite do add -lm flags, so it should be possible to make these tests build and run too?
I've added a way to add `-lm`. It seemed silly for all of 4 tests, but I also did it for `-fwrapv` and `-Wno-return-type` which fixed a few other tests. It has made the configuration a little more complex, but I think more manageable.
================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt:103-105
+ # Requires mmap
+ loop-2f.c
+ loop-2g.c
----------------
kristof.beyls wrote:
> I guess "Requiring mmap" could be split out into a separate cmake if-block that checks for "mmap" being available or a proxy for that?
I have done so.
================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt:134
+
+ # These have -fwrapv but still give runtime issues
+ 20040409-1w.c
----------------
kristof.beyls wrote:
> Is this because of a bug in -fwrapv handling in clang, or bugs in the test cases?
Almost certainly because my argument parsing code was wrong. I fixed it and now these work.
================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt:142
+
+ # This infinite loops with -O3
+ 930529-1.c
----------------
kristof.beyls wrote:
> Infinite loop because of undefined behaviour in the test case or due to a bug in clang?
I fixed the argument parsing code, and now it gets `-fwrapv` and doesn't infinite loop
================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt:161-162
+ file(GLOB RISCVTestsToSkip
+ # x86 Only
+ 990413-2.c
+
----------------
kristof.beyls wrote:
> If this is x86-only, maybe this should be modelled by enabling this test specifically when ARCH MATCHES "x86", instead of explicitly disabling it for all other arches?
> This is just a nit though - not sure if this would be straightforward to implement.
I did this. It's not too ugly.
================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt:164-166
+ # No support for __builtin_longjmp/__builtin_setjmp
+ built-in-setjmp.c
+ pr84521.c
----------------
kristof.beyls wrote:
> There are a few tests that are disabled in the generic disable-test section above that also have the comment "No support for __builin_longjmp/__builtin_setjmp".
> This suggests that all of these tests should either be in the generic section or in the RISC-V-specific section, but not split between them?
I revisited why the setjmp/longjump-related tests in these lists were failing, and some pass on x86 (so I want to keep them if possible), and some don't pass at all.
I've managed to split them into 3 groups. 1 is the stuff that is very undefined behavior, one is a test clang should almost certainly be passing, and the last two succeed on x86.
================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt:175
+
+list(APPEND CFLAGS "-g")
+
----------------
kristof.beyls wrote:
> I'm not sure if it's a good idea to add c flags unconditionally in this test specific CMakelists.txt.
> If you want debug info on your tests when invoking the test-suite, you can add -g to your CFLAGS there.
> Is there a reason why you always need to have debug info on these torture tests, but not on the other tests in the test-suite?
This was a mistake left in from testing. It's gone now.
================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/ieee/CMakeLists.txt:2-4
+ # Must link with libm
+ 20041213-1.c
+ mzero4.c
----------------
kristof.beyls wrote:
> Wouldn't it be possible to just add -lm to the link line for these tests and enable them?
I added a way of allowing us to specify -lm for these tests.
Repository:
rT test-suite
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66887/new/
https://reviews.llvm.org/D66887
More information about the llvm-commits
mailing list