This is an archive of the discontinued LLVM Phabricator instance.

[Clang test] Update to allow passing extra default clang arguments in use_clang
AbandonedPublic

Authored by guiand on Aug 11 2020, 2:35 PM.

Details

Summary

Prerequisite to a later patch that will add a default codegen option to the tests, for the sake of allowing clang to emit noundef without requiring changes to (nearly) every codegen test.

This adds a new clang substitution, '%clang_bin', that expands to the clang binary
with no extra arguments appended. This allows us
to replace uses of '%clang' where it's precisely required to use the pure binary,
and enables adding additional_flags to the test corpus.

Diff Detail

Event Timeline

guiand created this revision.Aug 11 2020, 2:35 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 11 2020, 2:35 PM
guiand requested review of this revision.Aug 11 2020, 2:35 PM
guiand updated this revision to Diff 284929.Aug 11 2020, 4:18 PM

Allows passing different extra arguments for different clang expansions

eugenis updated this revision to Diff 298693.Oct 16 2020, 11:35 AM

update tests

This revision is now accepted and ready to land.Oct 16 2020, 11:38 AM

Did we decide that we wanted this change then? I remember there being discussion around whether it's the right approach.

I don't like the %clang_bin substitution - imho it's unclear for the test authors when to use it instead of %clang, but I can't come up with a better idea.

Is it true that %clang_bin is only necessary when the automatically added -disable-noundef-analysis flag triggers an unused argument warning, or are there other reasons?

I wonder it can be avoided by

  • disable noundef analysis by default in cc1
  • always add -enable-noundef-analysis in the driver when invoking cc1
aqjune added a subscriber: aqjune.Dec 8 2020, 12:41 AM

I wonder it can be avoided by

  • disable noundef analysis by default in cc1
  • always add -enable-noundef-analysis in the driver when invoking cc1

Would this cause more tests that use %clang to be updated, such as test/CodeGen/mips-vector-return.c?
IIUC, the tests in this patch are okay to have their %clang replaced with %clang_bin because they are not checking function signatures.

I don't like the %clang_bin substitution - imho it's unclear for the test authors when to use it instead of %clang, but I can't come up with a better idea.

From a user's perspective, simply naming it as %clang_noundef would be clearer, IMO.
This new substitution is going to be used for the noundef checks in D81678 only anyway.

simply naming it as %clang_noundef would be clearer,

Hmm, yeah, that's a little better.

This new substitution is going to be used for the noundef checks in D81678 only anyway.

I'm not sure what you mean by this. The substitution is used ~380 times in this patch alone.
In the future, any attempt to use %clang -### or -%clang -cc1as will produce the same unused argument warning, and it is very unclear that is can be fixed with %clang_bin. Note that code search for "-disable-noundef-analysis" does NOT lead you to the definition of %clang_bin!

eugenis requested changes to this revision.Dec 8 2020, 4:22 PM

I wonder if we could just disable unused argument warning for both

-disable-noundef-analysis
-Xclang -disable-noundef-analysis

I don't see any precedents for this kind of thing in the code.

This revision now requires changes to proceed.Dec 8 2020, 4:22 PM
guiand added a comment.Dec 9 2020, 4:08 PM

This whole thing is a little unfortunate, but maybe a better substitution would be leaving %clang as referring to the pure clang binary, with default arguments. Then we can have a %clang_cc which may only be used for a standard C compiler invocation.

Returning to the main issue, adding a default argument to %clang breaks a number of tests. Generally these tests change the compiler mode such that a C compiler argument makes no sense. Consider %clang -cc1: this gets expanded to clang -Xclang -disable-noundef-analysis -cc1, which IIRC causes an error.
This isn't a problem with something like %clang_cc, because a C compiler flag would always be valid for that expansion.

IMO it's better to just one-and-done programatically add -Xclang -disable-noundef-analysis to all the tests' RUN lines. That way there are no hidden flags.

IMO it's better to just one-and-done programatically add -Xclang -disable-noundef-analysis to all the tests' RUN lines. That way there are no hidden flags.

If a script for this can be written and people who are working for the downstream project are happy with the script, this is the best approach IMO. It is clear and explicit.

This new substitution is going to be used for the noundef checks in D81678 only anyway.

I'm not sure what you mean by this. The substitution is used ~380 times in this patch alone.
In the future, any attempt to use %clang -### or -%clang -cc1as will produce the same unused argument warning, and it is very unclear that is can be fixed with %clang_bin. Note that code search for "-disable-noundef-analysis" does NOT lead you to the definition of %clang_bin!

I see, I was naively thinking the %clang -> %clang_bin changes were here to simply show that they are equivalent if function signatures are not checked. I thought %clang could be still used for those tests.
But they were here because the upcoming patch will otherwise make them raise unused argument warning.

IMO it's better to just one-and-done programatically add -Xclang -disable-noundef-analysis to all the tests' RUN lines. That way there are no hidden flags.

If a script for this can be written and people who are working for the downstream project are happy with the script, this is the best approach IMO. It is clear and explicit.

Yeah, I also think this would be the best.

guiand abandoned this revision.Mar 5 2021, 1:59 PM

We decided to go with a positive flag for enabling noundef, so I'm closing this.