[test-suite] Add GCC C Torture Suite

[lenary]

Summary

This patch adds support for testing clang/LLVM against the GCC Torture
suite.

This patch adds the CMake configuration and licence information for these tests. A follow-up patch will add the testcases themselves (which are too large to review, and included without modifications). They will be committed together.

[lenary]

• Support dg-options and compile tests properly
• Document checkout path

[lenary]

• Better support for dg-options
• Clarify rev in README

[asb]

Thanks for pushing this forwards Sam. For the RISC-V backend we've been using a downstream shell script for this for ages, and I've always felt it would be better if the torture suite could be used from the LLVM test suite. It's been a while since I went over the excluded tests, it's possible it would benefit from further review if all the masked tests really should be masked.

External/gcc-c-torture/CMakeLists.txt

38 ↗

(On Diff #217795)

I'm not 100% convinced how useful the compile-only tests are for the test-suite repo. I'd welcome other opinions.

External/gcc-c-torture/README

8 ↗

(On Diff #217795)

It would be useful to suggest an appropriate svn export command I think.

[lenary]

• Ignore target-specific dg-options
• remove support for compile-only tests (LNT wants to run things)
• Target-specific Files to Skip
• set lit config.single_source

[lenary]

Yeah, I need to review the list of tests to skip, which is on my list to do. I will probably initially review that list by testing on the x86-backend, and then letting other backends add their own lists of tests to skip (as we already have for the riscv backend).

External/gcc-c-torture/CMakeLists.txt

38 ↗

(On Diff #217795)

Yeah, my new feeling is that these are too hard to integrate into the test suite, because LNT really really really wants to have an executable to run. I've removed them in my most recent update.

[lenary]

External/gcc-c-torture/README

8 ↗

(On Diff #217795)

Alrighty

[lenary]

• Update gcc-c-torture README

[lewis-revill]

I'd just like to bring up that at Embecosm we have been in discussions with the GCC community for a while about an alternative approach to this style of blacklisting, where instead we add more fine grained procedures within the target-supports.exp file to allow us to annotate tests with things like /* { dg-require-effective-target builtin_setjmplngjmp } */. For now we have also added gcc_frontend and gcc_internals 'effective targets' to help with cases like tests which check for GCC-specific warning output, or GCC-specific optimization behaviour.

This work has been ongoing at https://github.com/embecosm/gcc-for-llvm-testing/.

This approach also works well for adding checks for backend-specific restrictions. For example checking for __attribute__((vector)) support.

[lenary]

• Update subdirectory logic in CMake
• Update test blacklist (using x86 for main blacklist)
• add notes on riscv blacklist
• take into account dg-additional-options

[lenary]

I have updated the patch to import the testcases, as this seems to be the
consensus on llvm-dev about what to do.

These testcases are all GPL'd code, but licences and origin have been documented
in the README.

I added these under the SingleSource directory, because each testcase is
standalone, but we use our own CMake macros because llvm_singlesource()
doesn't do enough for us.

[lenary]

• Remove unused file

[lenary]

This diff is huge, apologies.

The important parts are the CMakeLists.txt, and the files in SingleSource/Regression/C/gcc-c-torture/.

[lenary]

This was discussed on the LLVM-Dev list: http://lists.llvm.org/pipermail/llvm-dev/2019-September/134890.html

[kristof.beyls]

I think you also need to call out the new SingleSource/Regression/C/gcc-c-torture directory in the top-level LICENSE.TXT file as a directory containing non-LLVM licensed material (see bottom of the top-level LICENSE.TXT file).

SingleSource/Regression/C/gcc-c-torture/README

22–23

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?

[lenary]

Noted about the toplevel LICENCE.TXT - I shall update that shortly (hoping not to invalidate the links below).

SingleSource/Regression/C/gcc-c-torture/README

22–23

The exclusions happen in:

• SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt
• SingleSource/Regression/C/gcc-c-torture/execute/ieee/CMakeLists.txt

(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).

[kristof.beyls]

SingleSource/Regression/C/gcc-c-torture/README

22–23

Thanks a lot for the pointers!
I've added a range of small comments on those CMakeLists.

SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt

64–72

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?

92–94

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?

104–106

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?

108

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?

135

Is this because of a bug in -fwrapv handling in clang, or bugs in the test cases?

143

Infinite loop because of undefined behaviour in the test case or due to a bug in clang?

162–163

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.

165–167

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?

176

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?

SingleSource/Regression/C/gcc-c-torture/execute/ieee/CMakeLists.txt

3–5

Wouldn't it be possible to just add -lm to the link line for these tests and enable them?

[lenary]

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.

SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt

64–72

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.

92–94

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.

104–106

I have done so.

135

Almost certainly because my argument parsing code was wrong. I fixed it and now these work.

143

I fixed the argument parsing code, and now it gets -fwrapv and doesn't infinite loop

162–163

I did this. It's not too ugly.

165–167

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.

176

This was a mistake left in from testing. It's gone now.

SingleSource/Regression/C/gcc-c-torture/execute/ieee/CMakeLists.txt

3–5

I added a way of allowing us to specify -lm for these tests.

[lenary]

Address Review Feedback

• This patch no longer contains the tests, to make review easier. It contains all required licencing files, and the build configuration
• Updates to build configuration based on review feedback and extensive triage

[kristof.beyls]

Thanks Sam, I think this is starting to look good!
I just had a few more nit-picky comments.

Were you planning once you commit to quickly add skip lists for other architectures too so that all public bots for all architectures will keep on passing?

SingleSource/Regression/C/gcc-c-torture/CMakeLists.txt

2

s/int/into/ ?

27

s/recieves/receives/ ?

40

spell check disambuguator

44–51

I don't understand cmake much, I'm afraid.
My guess is that the CLANG_UNKNOWN_CFLAGS could only come from options encoded in specific test files, i.e. those that are being discovered through cmake function gcc_torture_dg_options_cflags.
If so, wouldn't it be better to only remove those options from the list of options that gcc_torture_dg_options_cflags finds?
IIUC, gcc_torture_dg_options_cflags currently modifies the global CFLAGS variable. Maybe a better design would be that it returns the list of options it finds without modifying the global CFLAGS variable. And do the filtering of flags that clang doesn't know inside gcc_torture_dg_options_cflags?
Let me repeat that I don't know much about the cmake language, so there may be idiomatic reasons why such a design would not be best.

SingleSource/Regression/C/gcc-c-torture/execute/LICENCE.txt

1 ↗

(On Diff #220884)

I think the filename for this file should be LICENSE.TXT to keep it consistent with other LICENSE.TXT files in the test-suite.

[khcheang]

Hello, we have integrated this patch to our test environment for a ri5cy target, and made the following changes. Please review. Thanks!

SingleSource/Regression/C/gcc-c-torture/execute/CMakeLists.txt

188

Added a group for testing of newlib-nano printf

# Tests that require newnlib Nano IO (-Wl,-u,_printf_float flag)
file(GLOB TestRequiresNanoIO CONFIGURE_DEPENDS
  920501-8.c
  930513-1.c
)

202

${MMapTests}

220

We added this exclude for rv32 target

1. __int128 is not supported on this target pr84748.c

232

${TestToSkip}

244

Added a group for newlib-nano tests

if(${File} IN_LIST TestRequiresNanoIO)
  list(APPEND MaybeLDFlags "-Wl,-u,_printf_float")
endif()

[lenary]

SingleSource/Regression/C/gcc-c-torture/CMakeLists.txt

44–51

I actually had to look up how scope works in CMake when I had a previous bug in this patch. I believe this is doing something reasonable.

In CMake, each subdirectory and each function are a new scope. They inherit the scope of the enclosing subdirectory/function, but new set calls (and things like append) apply only within the current scope.

So this function is not modifying a global CFLAGS, it's taking the per-subdirectory CFLAGS, adding some extra flags, and then setting the CFLAGS variable in the current function's scope.

I have decided to simplify the logic in gcc_torture_dg_options_cflags and set a variable in the parent scope with the dg-options cflags only, rather than updating CFLAGS. I also do the removal of the erroring/unknown cflags in the above function now.

[lenary]

• Address review feedback

[lenary]

@kristof.beyls @khcheang thank you both for the reviews! Sorry it has taken me a while to get back to you with an updated patch, I was unexpectedly away from my desk for two weeks.

[lenary]

• Update LDFlags for newlib nano io
• Expand TestToSkip in ieee foreach

[kristof.beyls]

Thanks for persevering Sam.

I think the latest changes look good.
I only have 2 more minor nits, see below. After those are addressed, I think this will be fine to commit.

Thanks!

SingleSource/Regression/C/gcc-c-torture/README

15–18

Given licensing information is also present in SingleSource/Regression/C/gcc-c-torture/execute/LICENSE.TXT and called out in the top-level LICENSE.TXT file in the test-suite, I don't think there's much value in repeating the information here too. Maybe best to delete this part?

SingleSource/Regression/C/gcc-c-torture/execute/LICENCE.TXT

1–2 ↗

(On Diff #223813)

it seems the file name here is still LICENCE.TXT rather than LICENSE.TXT, which would be preferred for consistency with the rest of the test-suite?

[lenary]

SingleSource/Regression/C/gcc-c-torture/README

15–18

Sure, deleted.

SingleSource/Regression/C/gcc-c-torture/execute/LICENCE.TXT

1–2 ↗

(On Diff #223813)

I'm really sorry about you having to note this twice. I must have been having a british/american spelling moment and missed the C/S difference. Updated.

[lenary]

• Update LICENSE.TXT spelling (American spelling this time)
• Remove Licensing info duplication from README

[kristof.beyls]

LGTM, thanks!

[lenary]

In D66887#1676164, @kristof.beyls wrote:

Were you planning once you commit to quickly add skip lists for other architectures too so that all public bots for all architectures will keep on passing?

Sorry, I missed addressing this.

There are two options:

1. I do nothing, people see that tests are failing and help me update the CMakeLists tests to skip. I think this would happen very quickly, as there would be failing tests.
2. I add a if(ARCH MATCHES "x86" OR ARCH MATCHES "riscv") around the add_subdirectory(...) in SingleSource/Regression/C/CMakeLists.txt. This will mean no errors, but I don't have a way of testing on other architectures so it could take a while for the torture suite to be supported by other architectures.

I'm not sure of the best way forward. If we want to avoid breakage: 2; if we're optimising for maximal test coverage: 1.

[lenary]

• Add architecture-testing guard to gcc c torture suite

[lenary]

• Rebase changes