[PATCH] D66887: [test-suite][WIP] Add GCC C Torture Suite as External Test Suite

Tue Sep 17 05:07:17 PDT 2019

kristof.beyls added inline comments.

================
Comment at: SingleSource/Regression/C/gcc-c-torture/README:21-22
+
+RISC-V: The list of tests to exclude on RISC-V was devised using glibc and qemu.
+You may have to exclude additional tests when running with newlib and/or spike.
----------------
lenary wrote:
> kristof.beyls wrote:
> > I was looking for how the test-exclusion mechanism works exactly, but I couldn't easily find an example.
> > According to the text above in the README, I think I should look at SingleSource/Regression/C/CMakeLists.txt, but I don't see any example of a test being skipped in there?
> > 
> > Could you point me to an example?
> The exclusions happen in:
> 
> * [SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt](https://reviews.llvm.org/differential/changeset/?ref=1597559)
> * [SingleSource/Regression/C/gcc-c-torture/execute/ieee/CMakeLists.txt](https://reviews.llvm.org/differential/changeset/?ref=1597830)
> 
> (I hope these links won't go out of date)
> 
> I apologise that the size of this diff has made these hard to find. I'm not sure of a way to split this diff into reasonable pieces (say, the build system in patch, and then the actual tests in another patch).
Thanks a lot for the pointers!
I've added a range of small comments on those CMakeLists.

================
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
----------------
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?

================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt:91-93
+  # Must link with libm
+  980709-1.c
+  float-floor.c
----------------
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?

================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt:103-105
+  # Requires mmap
+  loop-2f.c
+  loop-2g.c
----------------
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?

================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt:107
+
+  # Link error
+  medce-1.c
----------------
I think this comment could also be a bit more specific, if possible. Is the link error caused by a test problem or by a toolchain problem?

================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt:134
+
+  # These have -fwrapv but still give runtime issues
+  20040409-1w.c
----------------
Is this because of a bug in -fwrapv handling in clang, or bugs in the test cases?

================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt:142
+
+  # This infinite loops with -O3
+  930529-1.c
----------------
Infinite loop because of undefined behaviour in the test case or due to a bug in clang?

================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt:161-162
+  file(GLOB RISCVTestsToSkip
+    # x86 Only
+    990413-2.c
+
----------------
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.

================
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
----------------
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?

================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt:175
+
+list(APPEND CFLAGS "-g")
+
----------------
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?

================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/ieee/CMakeLists.txt:2-4
+  # Must link with libm
+  20041213-1.c
+  mzero4.c
----------------
Wouldn't it be possible to just add -lm to the link line for these tests and enable them?

Repository:
  rT test-suite

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66887/new/

https://reviews.llvm.org/D66887