[PATCH] D93395: [clang][cli] Remove -f[no-]trapping-math from -cc1 command line

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.

LGTM. I've just done a careful audit myself, and I'm now confident this patch is correct and that there is no latent bug -- that it's correct to ignore `-f*trapping-math` on the `-cc1` command-line since `-fp-exception-mode` will always have a value matching / superseding `-f*trapping-math`.

@SjoerdMeijer , please confirm you agree as well.



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2598-2601
         FPExceptionBehavior = "strict";
         FPModel = Val;
         FPContract = "off";
         TrappingMath = true;
----------------
`FPExceptionBehavior` and `TrappingMath` are set together here (in lock step).


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2626-2647
     case options::OPT_ftrapping_math:
       if (!TrappingMathPresent && !FPExceptionBehavior.empty() &&
           !FPExceptionBehavior.equals("strict"))
         // Warn that previous value of option is overridden.
         D.Diag(clang::diag::warn_drv_overriding_flag_option)
           << Args.MakeArgString("-ffp-exception-behavior=" + FPExceptionBehavior)
           << "-ftrapping-math";
----------------
Note this handling of `-ftrapping-math` in the driver, which uses it to set `FPExceptionBehaviour` correctly.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2698-2717
     // Validate and pass through -ffp-exception-behavior option.
     case options::OPT_ffp_exception_behavior_EQ: {
       StringRef Val = A->getValue();
       if (!TrappingMathPresent && !FPExceptionBehavior.empty() &&
           !FPExceptionBehavior.equals(Val))
         // Warn that previous value of option is overridden.
         D.Diag(clang::diag::warn_drv_overriding_flag_option)
----------------
Note that `-ffp-exception-behaviour` also sets `FPExceptionBehaviour` (and `TrappingMath`).


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2732-2733
       SignedZeros = false;
       TrappingMath = false;
       FPExceptionBehavior = "";
       break;
----------------
This will override `FPExceptionBehavior`, (wiping out any effect of `-ftrapping-math`), but that seems intentional and `TrappingMath` is set the same way.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2739-2740
       SignedZeros = true;
       TrappingMath = true;
       FPExceptionBehavior = "strict";
 
----------------
`FPExceptionBehavior` and `TrappingMath` are also set together here.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2829-2832
   if (TrappingMath) {
     // FP Exception Behavior is also set to strict
     assert(FPExceptionBehavior.equals("strict"));
+  }
----------------
Note: given the above, this assertion seems sufficient for confirming `-ftrapping-math` is effectively passed to `-cc1`.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3068-3075
-  if (Args.hasArg(OPT_ftrapping_math)) {
-    Opts.setFPExceptionMode(LangOptions::FPE_Strict);
-  }
-
-  if (Args.hasArg(OPT_fno_trapping_math)) {
-    Opts.setFPExceptionMode(LangOptions::FPE_Ignore);
-  }
----------------
As long as `-cc1` always contains `-ffp-exception-behavior` (which includes any adjustment from `-ftrapping-math`), then it seems correct to drop this as this patch is doing.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3080
   }
   Opts.setFPExceptionMode(FPEB);
 
----------------
SjoerdMeijer wrote:
> jansvoboda11 wrote:
> > SjoerdMeijer wrote:
> > > jansvoboda11 wrote:
> > > > The parsing of `OPT_ftrapping_math` and `OPT_fno_trapping_math` is immediately overwritten here.
> > > Yeah, I did some work in this area some years ago, but that's a few years ago, and then in addition to that, we are talking about option handling in Clang which I always find very confusing... Just saying I can't remember too many details at this point.
> > > 
> > > But starting with a first observation, you're saying that things are overwritten here, but they are different options. I.e., the deleted part honours `OPT_ftrapping_math`, and here exception modes are set based on `OPT_ffp_exception_behavior`. So, looks like there is some interaction here... Do we know how this should work?
> > I see. The thing is, before this patch, we call `Opts.setFPExceptionMode` whenever we see either `OPT_ftrapping_math` or `OPT_fno_trapping_math`.
> > 
> > But then, we **unconditionally** call `Opts.setFPExceptionMode(FPEB)` again here. That **always** overwrites what we previously did for `OPT_f[no_]trapping_math`, making that a dead code.
> > 
> > `Opts.setFPExceptionMode()` doesn't do anything fancy, just assigns the argument to the `Opts.FPExceptionModel` member.
> Ah yes, I actually missed that we always set `Opts.setFPExceptionMode(FPEB)` here! So that's clear now.
> 
> But looks like my previous question is still relevant how these options should work together? I now see that 
> `-ffp-exception-behavior` is a intel/microsoft special introduced in  D62731.
> 
> And while currently we ignore `-fftrapping-math` here, but that just seems like a bug, it looks like we actually need to handle it here too?
> 
I just did an audit (see my other comments), and it looks like `-ftrapping-math`'s behaviour is fully handled by the `-ffp-exception-mode` option in `-cc1`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93395/new/

https://reviews.llvm.org/D93395