[clang][driver] Add -ansi option to CompileOnly group

[tbaeder]

Summary

-ansi is documented as being the "same as -std=c89", but there are differences when passing it to a link:

clang -c test.c -o test.o

clang test.o -o test -std=c89   # Works, no problem

clang test.o -o test -ansi  # Warning about unused -ansi option

-std= is claimed in this case in clang/lib/Driver/Driver.cpp at the end of handleArguments() via Args.ClaimAllArgs(options::OPT_CompileOnly_Group). This does not include -ansi right now however, since it does not belong to the CompileOnly group.

Adding -ansi to said group makes sense since it's supposed to be an alias for -std=c89 and resolves this inconsistency.

[tbaeder]

Rebased on lastest main branch and added a few more reviewers from git blame'ing the Options.td file

[aaron.ballman]

This seems reasonable to me, but I wonder if there's a way we can add test coverage for the change?

[tbaeder]

I guess I could add a test for the example I posted, but I'm not sure of how much value that is. Or do you mean a test case that -ansi and -std=c89 behave the same in every situation?

[aaron.ballman]

In D93375#2479303, @tbaeder wrote:

I guess I could add a test for the example I posted, but I'm not sure of how much value that is. Or do you mean a test case that -ansi and -std=c89 behave the same in every situation?

I was thinking of a test that demonstrates that a given flag isn't passed along to the linker so that we could test that neither -std=whatever nor -ansi get passed along (to ensure we don't regress accidentally in the future). However, I'm not certain if that's a big ask or not.

[tbaeder]

That sounds good but it's different to what this patch is trying to accomplish, isn't it? -ansi already doesn't get passed to the linker.
E.g. with clang 10.0.1 without this patch:

~ ❯ clang test.o -o test -ansi -###
clang version 10.0.1 (Fedora 10.0.1-3.fc32)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
clang-10: warning: argument unused during compilation: '-ansi' [-Wunused-command-line-argument]
 "/usr/bin/ld" "--hash-style=gnu" "--build-id" "--eh-frame-hdr" "-m" "elf_x86_64" "-dynamic-linker" "/lib64/ld-linux-x86-64.so.2" "-o" "test" "/usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../lib64/crt1.o" "/usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../lib64/crti.o" "/usr/bin/../lib/gcc/x86_64-redhat-linux/10/crtbegin.o" "-L/usr/bin/../lib/gcc/x86_64-redhat-linux/10" "-L/usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../lib64" "-L/usr/bin/../lib64" "-L/lib/../lib64" "-L/usr/lib/../lib64" "-L/usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../.." "-L/usr/bin/../lib" "-L/lib" "-L/usr/lib" "test.o" "-lgcc" "--as-needed" "-lgcc_s" "--no-as-needed" "-lc" "-lgcc" "--as-needed" "-lgcc_s" "--no-as-needed" "/usr/bin/../lib/gcc/x86_64-redhat-linux/10/crtend.o" "/usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../lib64/crtn.o"

[aaron.ballman]

In D93375#2486165, @tbaeder wrote:

That sounds good but it's different to what this patch is trying to accomplish, isn't it? -ansi already doesn't get passed to the linker.
E.g. with clang 10.0.1 without this patch:

~ ❯ clang test.o -o test -ansi -###
clang version 10.0.1 (Fedora 10.0.1-3.fc32)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
clang-10: warning: argument unused during compilation: '-ansi' [-Wunused-command-line-argument]
 "/usr/bin/ld" "--hash-style=gnu" "--build-id" "--eh-frame-hdr" "-m" "elf_x86_64" "-dynamic-linker" "/lib64/ld-linux-x86-64.so.2" "-o" "test" "/usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../lib64/crt1.o" "/usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../lib64/crti.o" "/usr/bin/../lib/gcc/x86_64-redhat-linux/10/crtbegin.o" "-L/usr/bin/../lib/gcc/x86_64-redhat-linux/10" "-L/usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../lib64" "-L/usr/bin/../lib64" "-L/lib/../lib64" "-L/usr/lib/../lib64" "-L/usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../.." "-L/usr/bin/../lib" "-L/lib" "-L/usr/lib" "test.o" "-lgcc" "--as-needed" "-lgcc_s" "--no-as-needed" "-lc" "-lgcc" "--as-needed" "-lgcc_s" "--no-as-needed" "/usr/bin/../lib/gcc/x86_64-redhat-linux/10/crtend.o" "/usr/bin/../lib/gcc/x86_64-redhat-linux/10/../../../../lib64/crtn.o"

Hmm, I think I misunderstood what you had originally written then with:

-ansi is documented as being the "same as -std=c89", but there are differences when passing it to a link:

Regardless, the request is whether we can add test coverage to ensure we don't regress this behavior in the future.

[tbaeder]

Okay, I've added a test and made sure it fails before and succeeds after this patch.

[aaron.ballman]

LGTM, thank you for this!

[tbaeder]

And thank you for reviewing my patches :)

[aaron.ballman]

In D93375#2493480, @tbaeder wrote:

And thank you for reviewing my patches :)

Happy to help! Btw, do you need someone to commit them on your behalf?

[tbaeder]

Yes, I was gonna ask somebody else but if you have time, committing this one and https://reviews.llvm.org/D94478 would be nice

[aaron.ballman]

I've commit on your behalf in 348471575d9c24bbfb124ca5eac1589de075da88, thank you for the patch!

[tbaeder]

Thanks for the commits, Aaron. If you don't mind I have a short question: both commits don't seem to list the test cases, have they not been committed at all or are they filtered out in github and/or phabricator? Did I mess something up?

clang/include/clang/Driver/Options.td

def Xpreprocessor : Separate<["-"], "Xpreprocessor">, Group<Preprocessor_Group>,
   HelpText<"Pass <arg> to the preprocessor">, MetaVarName<"<arg>">;
 def X_Flag : Flag<["-"], "X">, Group<Link_Group>;
 def X_Joined : Joined<["-"], "X">, IgnoredGCCCompat;
 def Z_Flag : Flag<["-"], "Z">, Group<Link_Group>;
 // FIXME: All we do with this is reject it. Remove.
 def Z_Joined : Joined<["-"], "Z">;
 def all__load : Flag<["-"], "all_load">;
 def allowable__client : Separate<["-"], "allowable_client">;
-def ansi : Flag<["-", "--"], "ansi">;
+def ansi : Flag<["-", "--"], "ansi">, Group<CompileOnly_Group>;
 def arch__errors__fatal : Flag<["-"], "arch_errors_fatal">;
 def arch : Separate<["-"], "arch">, Flags<[NoXarchOption]>;
 def arch__only : Separate<["-"], "arch_only">;
 def a : Joined<["-"], "a">;
 def autocomplete : Joined<["--"], "autocomplete=">;
 def bind__at__load : Flag<["-"], "bind_at_load">;
 def bundle__loader : Separate<["-"], "bundle_loader">;
 def bundle : Flag<["-"], "bundle">;

clang/test/Driver/ansi.c

+// Check that -std=c89 and -ansi are both not forwarded to the linker
+// RUN: %clang -std=c89 %s -### -o %t.o 2>&1 | FileCheck --check-prefix=CHECK %s
+// CHECK: -std=c89
+// CHECK-NOT: -std=c89
+
+// Same check with -ansi (which is an alias for -std=c89)
+// RUN: %clang -ansi %s -### -o %t.o 2>&1 | FileCheck --check-prefix=ANSI %s
+// ANSI: -std=c89
+// ANSI-NOT: -ansi
+
+// Passing -std to linking and compiling is both valid
+// RUN: %clang -std=c89 %s -c -o %t1.o && %clang %t1.o -Werror -std=c89 -o %t1
+
+// Same for -ansi
+// RUN: %clang -ansi %s -c -o %t2.o && %clang %t2.o -Werror -ansi -o %t2
+
+int main(int argc, char **argv)
+{
+}