This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by pzheng on Jul 12 2020, 4:17 PM.

Details

Summary

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

Diff Detail

Event Timeline

pzheng created this revision.Jul 12 2020, 4:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2020, 4:17 PM

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?)

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.

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 updated this revision to Diff 277316.Jul 12 2020, 7:37 PM

Add a test case.

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 added inline comments.Jul 12 2020, 11:02 PM
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 added a comment.EditedJul 12 2020, 11:05 PM

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 added a comment.EditedJul 12 2020, 11:34 PM

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 marked 2 inline comments as done.Jul 15 2020, 7:31 PM

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 updated this revision to Diff 278361.Jul 15 2020, 7:37 PM

Add target triple used in test

pzheng abandoned this revision.Sep 4 2020, 3:01 PM