This is an archive of the discontinued LLVM Phabricator instance.

[test-suite] Unit tests to improve code coverage
ClosedPublic

Authored by afd on Oct 6 2020, 2:49 PM.

Details

Summary

Adds a batch of C tests that have been found to cover several hundred
lines of Clang/LLVM that are not covered by the unit and regression
tests of the main LLVM project, nor by the test suite when run with the
-O3 configuration.

The tests were originally generated using Csmith, and were then reduced
using C-Reduce. They have been checked for undefined behaviour-freedom
using Frama-C and CompCert, and manually checked to eliminate
implementation-defined behaviour.

Most of the new coverage achieved by these tests is in:

clang/lib/Analysis/CFG.cpp

Event Timeline

afd requested review of this revision.Oct 6 2020, 2:49 PM
afd created this revision.
afd added subscribers: lenary, anemet.

Hi @anemet and @lenary - I am keen to contribute these tests that improve on the code coverage achieved by the existing test suite. It's my first LLVM-related patch so I wasn't sure who the right folks to ask for reviews were, but I saw that you'd each done some recent reviews in the test suite so I wondered whether you might be suitable to review this, or to suggest other reviewers.

Thanks!

ccadar added a subscriber: ccadar.Oct 7 2020, 6:39 AM
lenary accepted this revision.Jan 14 2021, 10:09 AM

There's one warning to fix (which should be trivial), and then I'm happy for this to land. Given the warning, the fix will hopefully be obvious?

I looked at the compilation and execution of all of these under clang and gcc trunk (for x86-64), and they all agree with your provided outputs.

SingleSource/UnitTests/2020-01-06-coverage-003.c
7

Clang gives a warning here:

warning: result of comparison of constant 4073709551607 with expression of type 'int8_t' (aka 'signed char') is always true [-Wtautological-constant-out-of-range-compare]
This revision is now accepted and ready to land.Jan 14 2021, 10:09 AM
afd updated this revision to Diff 324693.Feb 18 2021, 10:11 AM

Fixed a compiler warning as spotted by @lenary

afd added a comment.Feb 18 2021, 10:13 AM

@lenary Thanks for the review! I have fixed the compiler warning that you spotted. I hope I did the right thing when I uploaded a revised patch (this is my first time working with the differential/arcanist workflow).

Are you able to land this for me?

Yeah, I can land this for you, I'll try and do that today.

afd added a comment.Feb 19 2021, 2:33 AM

Ace, thank you.

yroux added a subscriber: yroux.Feb 23 2021, 9:17 AM

Hi,

this commit broke ARM bots, because their data model is ILP32, thus it is not safe to use a long format specifier to print a 64 bits type.

los are available here: http://lab.llvm.org:8011/#/builders/59/builds/1227

Ithink you should either use inttypes.h PRId64 macro to print a int64_t or use %ji with a cast to intmax_t

I'll commit a fix that uses the inttypes macros.

@afd Some nits about changing the printfs. I have so far presumed that where a number is unsigned, you want me to print it as unsigned (using "%u" or equivalent via inttypes.h), and where it is signed, you want me to print it using "%i" (or equivalent). It raised some questions, below.

I'll have to get work to approve my updated patch before I can post it publicly, but that should be quick if I'm lucky.

SingleSource/UnitTests/2020-01-06-coverage-005.c
87

Here you format an unsigned number with %i (a signed format specifier), did you mean to? I'd sort of expect an explicit cast if you did.

There are other places where this is also the case, I'll comment on them.

SingleSource/UnitTests/2020-01-06-coverage-007.c
29

Also here

afd added a comment.Feb 24 2021, 7:18 AM

@lenary Thanks for looking at this. Indeed, those uses of %i where it should be %u were oversights.

Something I just realised is that I named the tests according to the date on which I created them, but actually they were created in October, not January - I must have made a typo when I named them 2020-01-06 instead of 2020-10-06. I don't suppose this matters much but if you're updating this patch and if it's easy to change the 01s to 10s that would be grand.

SingleSource/UnitTests/2020-01-06-coverage-003.c
7

I suppose this warning could be silenced by casting a to uint64_t. That has the potential to affect the new coverage that this test ended up hitting, and I don't see that it matters if a test leads to warnings like this being issued - we still need to know that the compiler behaves sensibly on code that triggers such warnings.

SingleSource/UnitTests/2020-01-06-coverage-005.c
87

Ah yes, that was an oversight thanks - so changing to %u woudl be great.

SingleSource/UnitTests/2020-01-06-coverage-007.c
29

Also an oversight - should be %u - thanks.

Cool, I have a patch to update to the right specifiers, and to update the reference output where it has changed (005 and 007 have unsigned numbers being printed as signed, and showing up in the current output as negative).

I'm not going to do the rename. I figured something was up when checking this stuff out, because I didn't think the patch was that old, but reasonably it just needs to be a unique name grouping, the date doesn't have to be 100% accurate, I feel.

yroux added a comment.Mar 1 2021, 12:30 AM

@lenary it'd be great if you can commit your fix today, the bots are red for long time