Skip to content

[Clang][Driver] Warn on complex number range incompatibility with GCC #144468

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+55 −0
Open

[Clang][Driver] Warn on complex number range incompatibility with GCC #144468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/include/clang/Basic/DiagnosticDriverKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,9 @@ def warn_drv_math_errno_enabled_after_veclib: Warning<
"math errno enabled by '%0' after it was implicitly disabled by '%1',"
" this may limit the utilization of the vector library">,
InGroup<MathErrnoEnabledWithVecLib>;
def warn_drv_gcc_incompatible_complex_range_override: Warning<
"combination of '%0' and '%1' results in complex number calculations incompatible with GCC">,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we perhaps add note diagnostics or a %select to the warning so we can explain how it's incompatible? I'm worried a user will get this diagnostic and say "okay, great, but... what does that mean and how do I fix that?" and being a bit stuck because they don't know what's incompatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback! I agree that a user wouldn't know how to handle this warning just by seeing it. How about adding the following note diagnostic and outputting it along with the incompatibility warning?
note: complex multiplication and division use different calculation methods than GCC. specify '%0' after '%1' for GCC compatibility

Implementing this would result in the following output:

$ clang test.cpp -fcx-fortran-rules -fcx-limited-range
clang: warning: overriding '-fcx-fortran-rules' option with '-fcx-limited-range' [-Woverriding-option]
clang: warning: combination of '-fcx-fortran-rules' and '-fcx-limited-range' results in complex number calculations incompatible with GCC [-Wgcc-compat]
clang: note: complex multiplication and division use different calculation methods than GCC. specify '-fcx-fortran-rules' after '-fcx-limited-range' for GCC compatibility

Copy link
Collaborator

Choose a reason for hiding this comment

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

A few thoughts:

  1. It's unfortunate that we emit two warnings for the same thing, especially by default. We really should try to find a way to emit only one warning, I think.
  2. The "different calculation methods" part of the note doesn't really tell the user all that much, but the important part is the "specify '-blah' after '-yada'" bit and that's good to keep.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand that it's better to emit a single warning message. In that case, how about simply adding the "specify" part to the end of the original warning message? At least I think users would understand what to do.
warning: combination of '-fcx-fortran-rules' and '-fcx-limited-range' results in complex number calculations incompatible with GCC. specify '-fcx-fortran-rules' after '-fcx-limited-range' for GCC compatibility [-Wgcc-compat]

However, this warning message might be redundant because it's long and displays the same option twice. It might be better to use the following instead:
warning: complex number calculation is incompatible with GCC. specify '-fcx-fortran-rules' after '-fcx-limited-range' for compatibility [-Wgcc-compat]

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand that it's better to emit a single warning message. In that case, how about simply adding the "specify" part to the end of the original warning message? At least I think users would understand what to do. warning: combination of '-fcx-fortran-rules' and '-fcx-limited-range' results in complex number calculations incompatible with GCC. specify '-fcx-fortran-rules' after '-fcx-limited-range' for GCC compatibility [-Wgcc-compat]

However, this warning message might be redundant because it's long and displays the same option twice. It might be better to use the following instead: warning: complex number calculation is incompatible with GCC. specify '-fcx-fortran-rules' after '-fcx-limited-range' for compatibility [-Wgcc-compat]

The logic here might get a bit odd because one warning is -Woverriding-option and the second is -Wgcc-compat. So I think what we want is:

// -Wgcc-compat -Wno-overriding-option
warning: complex number calculation is incompatible with GCC; specify '-fcx-fortran-rules' after '-fcx-limited-range' for compatibility

// -Woverriding-option -Wno-gcc-compat
warning: overriding '-fcx-fortran-rules' option with '-fcx-limited-range'

// -Wgcc-compat -Woverriding-option
warning: overriding '-fcx-fortran-rules' option with '-fcx-limited-range' and is incompatible with GCC; specify '-fcx-fortran-rules' after '-fcx-limited-range' for compatibility

though I'm not certain how easy or hard that would be to implement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// -Wgcc-compat -Wno-overriding-option
warning: complex number calculation is incompatible with GCC; specify '-fcx-fortran-rules' after '-fcx-limited-range' for compatibility

// -Woverriding-option -Wno-gcc-compat
warning: overriding '-fcx-fortran-rules' option with '-fcx-limited-range'

// -Wgcc-compat -Woverriding-option
warning: overriding '-fcx-fortran-rules' option with '-fcx-limited-range' and is incompatible with GCC; specify '-fcx-fortran-rules' after '-fcx-limited-range' for compatibility

Does this mean that if we are going to emit both -Woverriding-option and -Wgcc-compat warnings, the warning messages should be combined into one? If there is a way to determine whether the -Woverriding-option option is specified during driver processing (within RenderFloatingPointOptions in Clang.cpp), we might be able to implement this using %select or similar. However, I don't know how to determine if -Woverriding-option is specified.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean that if we are going to emit both -Woverriding-option and -Wgcc-compat warnings, the warning messages should be combined into one?

Yeah, I would combine it into one under the -Woverriding-option and use a %select so that the GCC compatibility parts are optionally emitted.

If there is a way to determine whether the -Woverriding-option option is specified during driver processing

IIRC, you can do that by adding an explicit compiler option for it, like:
https://github.com/llvm/llvm-project/blob/31bf9348fa10fc95c4a86ef81485652152bf9906/clang/include/clang/Driver/Options.td#L896C5-L896C17

llvm-project/clang/lib/Driver/ToolChains/Clang.cpp

Line 6455 in 31bf934

Args.hasFlag(options::OPT_Wdeprecated, options::OPT_Wno_deprecated,

InGroup<GccCompat>;

def note_drv_command_failed_diag_msg : Note<
"diagnostic msg: %0">;
Expand Down
19 changes: 19 additions & 0 deletions clang/lib/Driver/ToolChains/Clang.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2915,6 +2915,11 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
} else {
if (GccRangeComplexOption != "-fno-cx-limited-range")
EmitComplexRangeDiag(D, GccRangeComplexOption, "-fcx-limited-range");
// Warn about complex range option overrides incompatible with GCC.
if (GccRangeComplexOption == "-fcx-fortran-rules" ||
GccRangeComplexOption == "-fno-cx-fortran-rules")
D.Diag(clang::diag::warn_drv_gcc_incompatible_complex_range_override)
<< GccRangeComplexOption << A->getSpelling();
}
GccRangeComplexOption = "-fcx-limited-range";
LastComplexRangeOption = A->getSpelling();
Expand All @@ -2929,6 +2934,10 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
GccRangeComplexOption != "-fno-cx-fortran-rules")
EmitComplexRangeDiag(D, GccRangeComplexOption,
"-fno-cx-limited-range");
// Warn about complex range option overrides incompatible with GCC.
if (GccRangeComplexOption == "-fcx-fortran-rules")
D.Diag(clang::diag::warn_drv_gcc_incompatible_complex_range_override)
<< GccRangeComplexOption << A->getSpelling();
}
GccRangeComplexOption = "-fno-cx-limited-range";
LastComplexRangeOption = A->getSpelling();
Expand Down Expand Up @@ -3231,6 +3240,11 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
case options::OPT_ffast_math:
applyFastMath(true);
LastComplexRangeOption = A->getSpelling();
// Warn about complex range option overrides incompatible with GCC.
if (GccRangeComplexOption == "-fcx-fortran-rules" ||
GccRangeComplexOption == "-fno-cx-fortran-rules")
D.Diag(clang::diag::warn_drv_gcc_incompatible_complex_range_override)
<< GccRangeComplexOption << A->getSpelling();
if (A->getOption().getID() == options::OPT_Ofast)
LastFpContractOverrideOption = "-Ofast";
else
Expand All @@ -3255,6 +3269,11 @@ static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
Range != LangOptions::ComplexRangeKind::CX_Full)
EmitComplexRangeDiag(D, LastComplexRangeOption, "-fno-fast-math");
Range = LangOptions::ComplexRangeKind::CX_None;
// Warn about complex range option overrides incompatible with GCC.
if (GccRangeComplexOption == "-fcx-fortran-rules" ||
GccRangeComplexOption == "-fcx-limited-range")
D.Diag(clang::diag::warn_drv_gcc_incompatible_complex_range_override)
<< GccRangeComplexOption << A->getSpelling();
LastComplexRangeOption = "";
GccRangeComplexOption = "";
LastFpContractOverrideOption = "";
Expand Down
33 changes: 33 additions & 0 deletions clang/test/Driver/range.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,31 @@
// RUN: %clang -### -Werror --target=x86_64 -fno-fast-math -ffp-model=strict \
// RUN: -c %s 2>&1 | FileCheck --check-prefixes=FULL %s


// Test incompatibility of complex range override with GCC 14.2.0.

// RUN: %clang -### --target=x86_64 -fcx-limited-range -fno-fast-math \
// RUN: -c %s 2>&1 | FileCheck --check-prefixes=WARN_INCOMPAT1 %s

// RUN: %clang -### --target=x86_64 -fcx-fortran-rules -fcx-limited-range \
// RUN: -c %s 2>&1 | FileCheck --check-prefixes=WARN_INCOMPAT2 %s

// RUN: %clang -### --target=x86_64 -fcx-fortran-rules -fno-cx-limited-range \
// RUN: -c %s 2>&1 | FileCheck --check-prefixes=WARN_INCOMPAT3 %s

// RUN: %clang -### --target=x86_64 -fcx-fortran-rules -ffast-math \
// RUN: -c %s 2>&1 | FileCheck --check-prefixes=WARN_INCOMPAT4 %s

// RUN: %clang -### --target=x86_64 -fcx-fortran-rules -fno-fast-math \
// RUN: -c %s 2>&1 | FileCheck --check-prefixes=WARN_INCOMPAT5 %s

// RUN: %clang -### --target=x86_64 -fno-cx-fortran-rules -fcx-limited-range \
// RUN: -c %s 2>&1 | FileCheck --check-prefixes=WARN_INCOMPAT6 %s

// RUN: %clang -### --target=x86_64 -fno-cx-fortran-rules -ffast-math \
// RUN: -c %s 2>&1 | FileCheck --check-prefixes=WARN_INCOMPAT7 %s


// WARN1: warning: overriding '-fcx-limited-range' option with '-fcx-fortran-rules' [-Woverriding-option]
// WARN2: warning: overriding '-fno-cx-limited-range' option with '-fcx-fortran-rules' [-Woverriding-option]
// WARN3: warning: overriding '-fcx-fortran-rules' option with '-fno-cx-limited-range' [-Woverriding-option]
Expand All @@ -281,4 +306,12 @@
// CHECK-NOT: -complex-range=improved
// RANGE-NOT: -complex-range=

// WARN_INCOMPAT1: warning: combination of '-fcx-limited-range' and '-fno-fast-math' results in complex number calculations incompatible with GCC [-Wgcc-compat]
// WARN_INCOMPAT2: warning: combination of '-fcx-fortran-rules' and '-fcx-limited-range' results in complex number calculations incompatible with GCC [-Wgcc-compat]
// WARN_INCOMPAT3: warning: combination of '-fcx-fortran-rules' and '-fno-cx-limited-range' results in complex number calculations incompatible with GCC [-Wgcc-compat]
// WARN_INCOMPAT4: warning: combination of '-fcx-fortran-rules' and '-ffast-math' results in complex number calculations incompatible with GCC [-Wgcc-compat]
// WARN_INCOMPAT5: warning: combination of '-fcx-fortran-rules' and '-fno-fast-math' results in complex number calculations incompatible with GCC [-Wgcc-compat]
// WARN_INCOMPAT6: warning: combination of '-fno-cx-fortran-rules' and '-fcx-limited-range' results in complex number calculations incompatible with GCC [-Wgcc-compat]
// WARN_INCOMPAT7: warning: combination of '-fno-cx-fortran-rules' and '-ffast-math' results in complex number calculations incompatible with GCC [-Wgcc-compat]

// ERR: error: unsupported argument 'foo' to option '-fcomplex-arithmetic='
Loading