[Driver] Fix integrated_as definition by setting it as a DriverOption

[pzheng]

Summary

DriverOption seems to be accidentally removed from integrated_as definition in
commit e5158b5.

[dblaikie]

Is there missing test coverage for this in some way? (how does this patch change the observable behavior of clang? I guess --help would change?)

[pzheng]

Actually, this patch won't change --help because it just reduced some duplication by extracting the common part (" the integrated assembler") of the help message into the "help" of "OptOutFFlag". Sorry for the confusion.

The real fix here is to restore "integrated_as" as a DriverOption. Without this, when we have -fintegrated-as on a link line, it will be passed to the linker and cause the linker to fail because it is not a valid linker flag.

[dblaikie]

In D83648#2146472, @pzheng wrote:

Actually, this patch won't change --help because it just reduced some duplication by extracting the common part (" the integrated assembler") of the help message into the "help" of "OptOutFFlag". Sorry for the confusion.

The real fix here is to restore "integrated_as" as a DriverOption. Without this, when we have -fintegrated-as on a link line, it will be passed to the linker and cause the linker to fail because it is not a valid linker flag.

Sounds like that could be tested with a driver test (using -### to see what the linker command would be and validating that it doesn't include -fintegrated-as? - see other similar tests in clang/test/Driver, I think)

[pzheng]

Add a test case.

[MaskRay]

In D83648#2146472, @pzheng wrote:

Actually, this patch won't change --help because it just reduced some duplication by extracting the common part (" the integrated assembler") of the help message into the "help" of "OptOutFFlag". Sorry for the confusion.

The real fix here is to restore "integrated_as" as a DriverOption. Without this, when we have -fintegrated-as on a link line, it will be passed to the linker and cause the linker to fail because it is not a valid linker flag.

The description is not correct. The actual difference is:

static bool forwardToGCC(const Option &O) {
  // Don't forward inputs from the original command line.  They are added from
  // InputInfoList.
  return O.getKind() != Option::InputClass &&
         !O.hasFlag(options::DriverOption) && !O.hasFlag(options::LinkerInput);
}

If clang::driver::tools::gcc::Common is selected, DriverOption modified options are not forwarded to gcc.

clang -target x86_64-linux -fintegrated-as a.o '-###' => no -fintegrated-as

clang -target x86_64 -fintegrated-as a.o '-###' => "/usr/bin/gcc" "-fintegrated-as" "-m64" "-o" "a.out" "a.o"

In this sense, all -f options not recognized by GCC should have a DriverOption tag.... I don't think we have properly tagged all options.

clang/test/Driver/integrated-as.c

11

This test is incorrect. You need a specific target triple to select bare-metal like GCC driver.

[MaskRay]

clang/test/Driver/integrated-as.c

11

There are so many clang specific options. I think we just need a centralized test file to test options in patch, instead of adding random -NOT check lines to random files.

[MaskRay]

Actually, the way we tag options recognized by GCC is wrong. There are few options which can be used on the GCC linking command line. So we should tag the options which can be used, rather than options which can't be used.

[MaskRay]

Created http://lists.llvm.org/pipermail/cfe-dev/2020-July/066245.html [cfe-dev] Usage of clang::driver::options::DriverOption (-Xarch_ & gcc toolchain)
to ask about the use case.

When I get time, I'll work on a patch fixing the whole class of options instead of just -fintegrated-as. I will use OPT_Link_Group and add some extra options in the new group: GCCLinkOption.

[pzheng]

In D83648#2146606, @MaskRay wrote:

Created http://lists.llvm.org/pipermail/cfe-dev/2020-July/066245.html [cfe-dev] Usage of clang::driver::options::DriverOption (-Xarch_ & gcc toolchain)
to ask about the use case.

When I get time, I'll work on a patch fixing the whole class of options instead of just -fintegrated-as. I will use OPT_Link_Group and add some extra options in the new group: GCCLinkOption.

Sounds like I should remove the test from this patch since you will fix a class of options later?

clang/test/Driver/integrated-as.c

11

Thanks for pointing this out. Will update the triple.

11

Can you suggest a file to add the test?

[pzheng]

Add target triple used in test

[MaskRay]

Fixed by 6a75496836ea14bcfd2f4b59d35a1cad4ac58cee :)

clang/include/clang/Driver/Options.td

 def whatsloaded : Flag<["-"], "whatsloaded">;
 def whyload : Flag<["-"], "whyload">;
 def w : Flag<["-"], "w">, HelpText<"Suppress all warnings">, Flags<[CC1Option]>;
 def x : JoinedOrSeparate<["-"], "x">, Flags<[DriverOption,CC1Option]>,
   HelpText<"Treat subsequent input files as having type <language>">,
   MetaVarName<"<language>">;
 def y : Joined<["-"], "y">;
 
-defm integrated_as : OptOutFFlag<"integrated-as", "Enable the integrated assembler", "Disable the integrated assembler">;
+defm integrated_as : OptOutFFlag<"integrated-as", "Enable", "Disable", " the integrated assembler", [DriverOption]>;
 
 def fintegrated_cc1 : Flag<["-"], "fintegrated-cc1">,
                       Flags<[CoreOption, DriverOption]>, Group<f_Group>,
                       HelpText<"Run cc1 in-process">;
 def fno_integrated_cc1 : Flag<["-"], "fno-integrated-cc1">,
                          Flags<[CoreOption, DriverOption]>, Group<f_Group>,
                          HelpText<"Spawn a separate process for each cc1">;
 

clang/test/Driver/integrated-as.c

+// REQUIRES: x86-registered-target
 // RUN: %clang -### -c -save-temps -integrated-as %s 2>&1 | FileCheck %s
 
 // CHECK: cc1as
 // CHECK: -mrelax-all
 
 // RUN: %clang -### -fintegrated-as -c -save-temps %s 2>&1 | FileCheck %s -check-prefix FIAS
 
 // FIAS: cc1as
 
+// RUN: %clang -target x86_64 -### -fintegrated-as %s 2>&1 | FileCheck %s -check-prefix FIAS-LINK
+
+// FIAS-LINK-NOT: -fintegrated-as
+
 // RUN: %clang -target none -### -fno-integrated-as -S %s 2>&1 \
 // RUN:     | FileCheck %s -check-prefix NOFIAS
 
 // NOFIAS-NOT: cc1as
 // NOFIAS: -cc1
 // NOFIAS: "-fno-verbose-asm"
 // NOFIAS: -no-integrated-as
 
 // RUN: %clang -target arm-linux-androideabi -### \
 // RUN:   -integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-ANDROID %s
 // CHECK-ARM-ANDROID: "-mnoexecstack"