Skip to content

Conversation

@xxxbxxx
Copy link
Contributor

@xxxbxxx xxxbxxx commented Dec 11, 2023

fix zig cc -fstack-protector

was failing since 53f74d6, with error: unable to create compilation: StackProtectorUnsupportedByTarget

also fixes zig build-exe that no longer enabled stack-protector by default when compiling c files.

closes #18009
closes #18114

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this.

Let's discuss here but then I'll need to incorporate this change into #18160 which directly conflicts.

I don't think this is the correct fix. use_llvm only indicates whether the LLVM backend will be used for compiling .zig code. It should have no effect on the C frontend.

Eyeballing the diff from 53f74d6, it looks to me like the regression came from target_util.supportsStackProtector correctly noticing which zig backend is being used to generate code, while the logic calling that function incorrectly assumes that .zig code is being compiled, when in reality it might be only C code being compiled.

So I think the correct fix is also to introduce another "supports stack protector" function to target_util for C compilation, and this one will take the c_frontend as a parameter. Then the logic will look something like this:

if (have_zcu and !target_util.zigSupportsStackProtector(target, zig_backend)) {
    // then disable it and error if explicitly requested
}
if (have_any_c_files and !target_util.cSupportsStackProtector(target, c_frontend, zig_backend)) {
    // then disable it and error if explicitly requested
}

have_zcu is "have a zig compilation unit" i.e. there is any .zig code being compiled. src/Module.zig will be renamed to Zcu soon.

@xxxbxxx
Copy link
Contributor Author

xxxbxxx commented Dec 11, 2023

Thanks for looking into this.

Let's discuss here but then I'll need to incorporate this change into #18160 which directly conflicts.

ok! I'll update this once #18160 has landed.

I don't think this is the correct fix. use_llvm only indicates whether the LLVM backend will be used for compiling .zig code. It should have no effect on the C frontend.

yeah I wasn't so sure about the meaning of use_llvm. thanks for clarifying.

andrewrk added a commit that referenced this pull request Dec 14, 2023
Commit 97e2389 regressed this behavior
because it made target_util.supportsStackProtector *correctly*
notice which zig backend is being used to generate code, while the
logic calling that function *incorrectly assumed* that .zig code is being
compiled, when in reality it might be only C code being compiled.

This commit adjusts the option resolution logic for stack protector so
that it takes into account the zig backend only if there is a zig
compilation unit. A separate piece of logic checks whether clang
supports stack protector for a given target.

closes #18009
closes #18114
closes #18254
andrewrk added a commit that referenced this pull request Dec 19, 2023
Commit 97e2389 regressed this behavior
because it made target_util.supportsStackProtector *correctly*
notice which zig backend is being used to generate code, while the
logic calling that function *incorrectly assumed* that .zig code is being
compiled, when in reality it might be only C code being compiled.

This commit adjusts the option resolution logic for stack protector so
that it takes into account the zig backend only if there is a zig
compilation unit. A separate piece of logic checks whether clang
supports stack protector for a given target.

closes #18009
closes #18114
closes #18254
andrewrk added a commit that referenced this pull request Dec 25, 2023
Commit 97e2389 regressed this behavior
because it made target_util.supportsStackProtector *correctly*
notice which zig backend is being used to generate code, while the
logic calling that function *incorrectly assumed* that .zig code is being
compiled, when in reality it might be only C code being compiled.

This commit adjusts the option resolution logic for stack protector so
that it takes into account the zig backend only if there is a zig
compilation unit. A separate piece of logic checks whether clang
supports stack protector for a given target.

closes #18009
closes #18114
closes #18254
andrewrk added a commit that referenced this pull request Dec 27, 2023
Commit 97e2389 regressed this behavior
because it made target_util.supportsStackProtector *correctly*
notice which zig backend is being used to generate code, while the
logic calling that function *incorrectly assumed* that .zig code is being
compiled, when in reality it might be only C code being compiled.

This commit adjusts the option resolution logic for stack protector so
that it takes into account the zig backend only if there is a zig
compilation unit. A separate piece of logic checks whether clang
supports stack protector for a given target.

closes #18009
closes #18114
closes #18254
andrewrk added a commit that referenced this pull request Dec 28, 2023
Commit 97e2389 regressed this behavior
because it made target_util.supportsStackProtector *correctly*
notice which zig backend is being used to generate code, while the
logic calling that function *incorrectly assumed* that .zig code is being
compiled, when in reality it might be only C code being compiled.

This commit adjusts the option resolution logic for stack protector so
that it takes into account the zig backend only if there is a zig
compilation unit. A separate piece of logic checks whether clang
supports stack protector for a given target.

closes #18009
closes #18114
closes #18254
andrewrk added a commit that referenced this pull request Dec 29, 2023
Commit 97e2389 regressed this behavior
because it made target_util.supportsStackProtector *correctly*
notice which zig backend is being used to generate code, while the
logic calling that function *incorrectly assumed* that .zig code is being
compiled, when in reality it might be only C code being compiled.

This commit adjusts the option resolution logic for stack protector so
that it takes into account the zig backend only if there is a zig
compilation unit. A separate piece of logic checks whether clang
supports stack protector for a given target.

closes #18009
closes #18114
closes #18254
andrewrk added a commit that referenced this pull request Dec 29, 2023
Commit 97e2389 regressed this behavior
because it made target_util.supportsStackProtector *correctly*
notice which zig backend is being used to generate code, while the
logic calling that function *incorrectly assumed* that .zig code is being
compiled, when in reality it might be only C code being compiled.

This commit adjusts the option resolution logic for stack protector so
that it takes into account the zig backend only if there is a zig
compilation unit. A separate piece of logic checks whether clang
supports stack protector for a given target.

closes #18009
closes #18114
closes #18254
andrewrk added a commit that referenced this pull request Jan 1, 2024
Commit 97e2389 regressed this behavior
because it made target_util.supportsStackProtector *correctly*
notice which zig backend is being used to generate code, while the
logic calling that function *incorrectly assumed* that .zig code is being
compiled, when in reality it might be only C code being compiled.

This commit adjusts the option resolution logic for stack protector so
that it takes into account the zig backend only if there is a zig
compilation unit. A separate piece of logic checks whether clang
supports stack protector for a given target.

closes #18009
closes #18114
closes #18254
andrewrk added a commit that referenced this pull request Jan 2, 2024
Commit 97e2389 regressed this behavior
because it made target_util.supportsStackProtector *correctly*
notice which zig backend is being used to generate code, while the
logic calling that function *incorrectly assumed* that .zig code is being
compiled, when in reality it might be only C code being compiled.

This commit adjusts the option resolution logic for stack protector so
that it takes into account the zig backend only if there is a zig
compilation unit. A separate piece of logic checks whether clang
supports stack protector for a given target.

closes #18009
closes #18114
closes #18254
@andrewrk andrewrk closed this in 43720be Jan 2, 2024
@xxxbxxx xxxbxxx deleted the zig-cc-llvm branch August 4, 2024 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

building a c/c++ file with zig build-exe no longer enables stack-protector zig cc -c -fstack-protector stopped working

2 participants