Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zig cc: zig c++: add support for -x and --language arguments. #9029

Closed

Conversation

redj
Copy link
Contributor

@redj redj commented Jun 7, 2021

When using zig cc and zig c++ allow passing -x / --language to clang to specify the programming language of a source file.

This is the long overdue re-opening of #6631. Sorry for the delay.
This required #9022 in order to run the update_clang_options tool.

src/main.zig Outdated
@@ -3329,6 +3346,7 @@ pub const ClangArgIterator = struct {
red_zone,
no_red_zone,
strip,
treat_as_language,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
treat_as_language,
force_language,

treat_as_file_ext = .cpp;
} else {
treat_as_file_ext = .unknown;
fatal("unsupported value '{s}' for -x or --language option", .{it.only_arg});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not true, there are many other valid cases that are not handled.

Copy link
Contributor Author

@redj redj Jun 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LemonBoy, are you suggesting I change the error message or support all the valid options?

This would be the valid options:

assembler-with-cpp
c
cpp-output
cl
cuda
cuda-cpp-output
hip
hip-cpp-output
objective-c
objective-c-cpp-output
objc-cpp-output
c++
c++-cpp-output
objective-c++
objective-c++-cpp-output
objc++-cpp-output
c-header
cl-header
objective-c-header
c++-module
ast
pcm
ir

the FileExt enum only has the following values:

    c,
    cpp,
    h,
    ll,
    bc,
    assembly,
    shared_library,
    object,
    static_library,
    zig,
    unknown,

And if we go the whole way, I'm not sure if this is relevant?

      // Follow gcc behavior and treat as linker input for invalid -x
      // options. Its not clear why we shouldn't just revert to unknown; but
      // this isn't very important, we might as well be bug compatible.
      if (!InputType) {
        Diag(clang::diag::err_drv_unknown_language) << A->getValue();
        InputType = types::TY_Object;
      }

Off-Topic:

I'm wondering if this is an LLVM bug?

TYPE("ir",                       LLVM_IR,      INVALID,         "ll",     phases::Compile, phases::Backend, phases::Assemble, phases::Link)
TYPE("ir",                       LLVM_BC,      INVALID,         "bc",     phases::Compile, phases::Backend, phases::Assemble, phases::Link)

LLVM_IR and LLVM_BC are supposed to be user specifiable but using ir will always select LLVM_IR. Should that 2nd line be this instead?

TYPE("bc",                       LLVM_BC,      INVALID,         "bc",     phases::Compile, phases::Backend, phases::Assemble, phases::Link)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you suggesting I change the error message or support all the valid options?

The latter, adding more valid extensions is also good for plain zig as, at the moment, it will outright reject any objective-c file.

src/main.zig Outdated
@@ -612,6 +612,7 @@ fn buildOutputType(
var subsystem: ?std.Target.SubSystem = null;
var major_subsystem_version: ?u32 = null;
var minor_subsystem_version: ?u32 = null;
var treat_as_file_ext: Compilation.FileExt = .unknown;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an optional switch that overrides the autodetection mechanism, using an optional type is better than initializing this variable in a known-invalid state.

Suggested change
var treat_as_file_ext: Compilation.FileExt = .unknown;
var force_language: ?Compilation.FileExt = null;

src/main.zig Outdated
if (treat_as_file_ext == .unknown) {
file_ext = Compilation.classifyFileExt(mem.spanZ(it.only_arg));
} else {
file_ext = treat_as_file_ext;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct in the presence of both object and source inputs? I think you only want the option to influence the latter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LemonBoy, according to https://clang.llvm.org/docs/ClangCommandLineReference.html :

-x<language>, --language <arg>, --language=<arg>¶
Treat subsequent input files as having type <language>

... so I would think you're not supposed to specify any other kind of inputs after a -x / --language option?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was a question for you. I've just checked and clang is blindly treating any input (even binary ones) as source code, this logic is correct.

src/main.zig Outdated
@@ -1125,7 +1126,12 @@ fn buildOutputType(
try clang_argv.appendSlice(it.other_args);
},
.positional => {
const file_ext = Compilation.classifyFileExt(mem.spanZ(it.only_arg));
var file_ext: Compilation.FileExt = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit, don't initialize variables in invalid state. You can easily use a block or, if possible, the if expression to initialize it:

const file_ext = if (treat_as_file_ext == .unknown) X else Y;
// or
const file_ext = blk: { if (treat_as_file_ext == .unknown) break :blk X else break :blk Y };

@redj redj marked this pull request as draft June 8, 2021 11:31
@Vexu Vexu added the frontend Tokenization, parsing, AstGen, Sema, and Liveness. label Jun 14, 2021
@Vexu
Copy link
Member

Vexu commented Aug 20, 2021

What is the state of this pr? All of the reviews seem to be addressed and there have been no changes in two months yet it is still marked as a draft.

@andrewrk
Copy link
Member

andrewrk commented Oct 5, 2021

Closing since this was opened as a draft and untouched for over 30 days. Please feel free to open a fresh PR if you wish to take this on again.

@andrewrk andrewrk closed this Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Tokenization, parsing, AstGen, Sema, and Liveness.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants