Page MenuHomePhabricator

Unit tests to improve code coverage with IR test too, also for the test in request D118234
Needs ReviewPublic

Authored by arindamsharma8 on Jan 26 2023, 7:58 AM.

Details

Reviewers
afd
lenary
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 our fuzzer, and were then reduced
using C-Reduce. These test have been checked for undefined behaviour-freedom
using Frama-C and code sanitisers, manually checked to eliminate
implementation-defined behaviour.

Most of the new coverage achieved by these 7 C code tests is in:
llvm-project/clang/lib/AST (2 testcases)
llvm-project/clang/lib/Sema (2 testcase)
llvm-project/llvm/lib/Analysis (1 testcase)
llvm-project/llvm/lib/CodeGen (4 testcases)
llvm-project/llvm/lib/Transforms(1 testcase)

For the 13 IR tests (of the .c tests here and in request D118234):
llvm-project/llvm/lib/Transforms/ (4 testcases)
llvm-project/llvm/lib/IR/ (1 testcase)
llvm-project/llvm/lib/Support/ (2 testcases)
llvm-project/clang/lib/AST (5 testcases)
llvm-project/clang/lib/Sema (2 testcase)
llvm-project/llvm/lib/CodeGen (4 testcases)

Diff Detail

Event Timeline

arindamsharma8 created this revision.Jan 26 2023, 7:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2023, 7:58 AM
arindamsharma8 requested review of this revision.Jan 26 2023, 7:58 AM
karineek added a subscriber: karineek.
afd added a comment.Feb 10 2023, 9:16 AM

I reviewed the C programs, and they look good except for one request for change and one query.

For the cases where there are C programs it makes sense to me that IR tests are also contributed.

What I'm unsure about is why IR tests are contributed without C programs. Are these the IR tests corresponding to previously-contributed C test cases?

SingleSource/UnitTests/testcase-Float2Int.c
3

Could the blank space be changed to a numeric value? To understand the code below, one needs to know what a blank space char value looks like when cast to double.

SingleSource/UnitTests/testcase-SemaStmt-1.c
5

Is it definitely OK for __builtin_expect to use a side-effecting operation as its second argument?

If so then this looks fine.

But if it would be OK for an implementation to #define the __builtin_expect expression to nothing, then this test would yield a different result, so I'm a bit unsure.

@lenary: I assume the .ll files shall go to the tests in the llvm project (e.g. llvm-project/tree/main/llvm/test/CodeGen/X86), right?
Also, how shall we build these? (we did not use emit-llvm flag, but maybe should have?)

SingleSource/UnitTests/testcase-Float2Int.c
3

Yes, it is not changing the coverage. I will edit it to 32.

SingleSource/UnitTests/testcase-SemaStmt-1.c
5

When moving the ++d outside (and just using d), we don't get the coverage. I think I will remove this test and maybe later we will find a test that covers that function.

@lenary: I assume the .ll files shall go to the tests in the llvm project (e.g. llvm-project/tree/main/llvm/test/CodeGen/X86), right?
Also, how shall we build these? (we did not use emit-llvm flag, but maybe should have?)

If you're going to put the llvm ir tests in-tree, then they should not rely on being linked/executed as they currently are. But it does sound like a better place for them given they are x86-specific, rather than target-independent (while LLVM IR semantics are portable, the IR generated by clang for different targets ends up being different for a variety of reasons).

I don't work on the x86 backend, so you'd have to work out a better person to review the test additions there.

Addresses @afd comments and update testcases accordingly.
Thanks for the feedback @afd.
@lenary we will move the IR tetscases into the appropriate location and make them either target independent or have a separate for each target x-86, ARM etc.

We are closing this review request due to the added IR tests being mixed with the original C single source tests. We will create 2 separate requests for these.

Arindam

arindamsharma8 changed the visibility from "Public (No Login Required)" to "No One".Thu, May 18, 3:22 AM
arindamsharma8 changed the edit policy from "All Users" to "No One".
arindamsharma8 changed the visibility from "No One" to "Public (No Login Required)".Thu, May 18, 3:27 AM
arindamsharma8 changed the edit policy from "No One" to "All Users".