[Clang] Make template instantiations respect pragma FENV_ACCESS state at point-of-definition

[zahiraam]

Summary

https://github.com/llvm/llvm-project/issues/63063

See also: https://godbolt.org/z/aYME8KTdn

[tbaeder]

Has the test case been obfuscated on purpose?

[zahiraam]

In D152818#4417644, @tbaeder wrote:

Has the test case been obfuscated on purpose?

This is a smaller reproducible from creduce. Didn't know if it would be OK to have an include in a LIT test?

[tbaeder]

In D152818#4417649, @zahiraam wrote:

In D152818#4417644, @tbaeder wrote:

Has the test case been obfuscated on purpose?

This is a smaller reproducible from creduce. Didn't know if it would be OK to have an include in a LIT test?

Okay, not sure if we have a policy regarding that. Just looking at it, I don't think anyone understands what it's doing.

[zahiraam]

In D152818#4417683, @tbaeder wrote:

In D152818#4417649, @zahiraam wrote:

In D152818#4417644, @tbaeder wrote:

Has the test case been obfuscated on purpose?

This is a smaller reproducible from creduce. Didn't know if it would be OK to have an include in a LIT test?

Okay, not sure if we have a policy regarding that. Just looking at it, I don't think anyone understands what it's doing.

It's definitely related to the use of the pragma and the throw function. I could put the "fstream" header in an Inputs folder? I see an #include <stdio.h> in Analysis/z3/Inputs/MockZ3_solver_check.c.

[aaron.ballman]

In D152818#4417704, @zahiraam wrote:

In D152818#4417683, @tbaeder wrote:

In D152818#4417649, @zahiraam wrote:

In D152818#4417644, @tbaeder wrote:

Has the test case been obfuscated on purpose?

This is a smaller reproducible from creduce. Didn't know if it would be OK to have an include in a LIT test?

Okay, not sure if we have a policy regarding that. Just looking at it, I don't think anyone understands what it's doing.

It's definitely related to the use of the pragma and the throw function. I could put the "fstream" header in an Inputs folder? I see an #include <stdio.h> in Analysis/z3/Inputs/MockZ3_solver_check.c.

No, we don't want to include actual STL header content in the lit tests (we will mock up interfaces as needed, but they're usually uninteresting). It's worth noting that the original test case in your godbolt link has a quite different stack trace than your reduced example (they still end up in the same function though), so are there two different bugs? https://godbolt.org/z/Yaf5185cE

I spent some time reducing the test case down to:

void g(const char *);

template <class>
struct q {
  static void r() { g(""); }
};

#pragma STDC FENV_ACCESS ON
void a() { q<int>::r(); }

so there's nothing to do with throw expressions as the summary says. As best I can tell, this has something to do with template instantiation (making q no longer a template or calling g() directly both cause the issue to go away). I'd like to know a bit more about why the changes to the assertion are correct.

clang/lib/CodeGen/CodeGenFunction.cpp

161–164

Constructors and destructors are function declarations, so those no longer need an explicit check.

[rjmccall]

I'd guess we're not resetting the pragma state appropriately when instantiating templates. Expressions should be governed by the pragmas in scope at the point of definition, not at the point of instantiation.

[nicolerabjohn]

I've opened an issue (https://github.com/llvm/llvm-project/issues/63063) where we hit the same assertion. There's a smaller test case, and we've done enough investigation that we understand the problem. I'd be happy to discuss over on the issue as well.

[zahiraam]

In D152818#4421064, @nicolerabjohn wrote:

I've opened an issue (https://github.com/llvm/llvm-project/issues/63063) where we hit the same assertion. There's a smaller test case, and we've done enough investigation that we understand the problem. I'd be happy to discuss over on the issue as well.

Thanks. That sounds good!

[rjmccall]

Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper?

[nicolerabjohn]

In D152818#4442116, @rjmccall wrote:

Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper?

It does not solve the problem, at least for my test case (linked in https://github.com/llvm/llvm-project/issues/63063) - we still hit the assertion.

[zahiraam]

In D152818#4456483, @nicolerabjohn wrote:

In D152818#4442116, @rjmccall wrote:

Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper?

It does not solve the problem, at least for my test case (linked in https://github.com/llvm/llvm-project/issues/63063) - we still hit the assertion.

Sorry! I will not be able to work on this until about September! I haven't tried to reproduce the issue with the test case in the link above.

[aaron.ballman]

In D152818#4456510, @zahiraam wrote:

In D152818#4456483, @nicolerabjohn wrote:

In D152818#4442116, @rjmccall wrote:

Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper?

It does not solve the problem, at least for my test case (linked in https://github.com/llvm/llvm-project/issues/63063) - we still hit the assertion.

Sorry! I will not be able to work on this until about September! I haven't tried to reproduce the issue with the test case in the link above.

We branch for the 17 release at the end of July, so I'm wondering whether there's anything we need to revert related to this? It looks like this assertion started firing in Clang 12.0.0 (https://cexplorer.testlabs.pro/z/Wh9o8W), so this doesn't seem to be a regression, but confirmation would be appreciated.

[zahiraam]

In D152818#4456717, @aaron.ballman wrote:

In D152818#4456510, @zahiraam wrote:

In D152818#4456483, @nicolerabjohn wrote:

In D152818#4442116, @rjmccall wrote:

Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper?

It does not solve the problem, at least for my test case (linked in https://github.com/llvm/llvm-project/issues/63063) - we still hit the assertion.

Sorry! I will not be able to work on this until about September! I haven't tried to reproduce the issue with the test case in the link above.

We branch for the 17 release at the end of July, so I'm wondering whether there's anything we need to revert related to this? It looks like this assertion started firing in Clang 12.0.0 (https://cexplorer.testlabs.pro/z/Wh9o8W), so this doesn't seem to be a regression, but confirmation would be appreciated.

The code that is generating the assertion has been introduced by this patch: https://reviews.llvm.org/D80462
I can checkout that commit and see if the test case fails with it? Would that be a good experiment to do?

[aaron.ballman]

In D152818#4456797, @zahiraam wrote:

In D152818#4456717, @aaron.ballman wrote:

In D152818#4456510, @zahiraam wrote:

In D152818#4456483, @nicolerabjohn wrote:

In D152818#4442116, @rjmccall wrote:

Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper?

It does not solve the problem, at least for my test case (linked in https://github.com/llvm/llvm-project/issues/63063) - we still hit the assertion.

Sorry! I will not be able to work on this until about September! I haven't tried to reproduce the issue with the test case in the link above.

We branch for the 17 release at the end of July, so I'm wondering whether there's anything we need to revert related to this? It looks like this assertion started firing in Clang 12.0.0 (https://cexplorer.testlabs.pro/z/Wh9o8W), so this doesn't seem to be a regression, but confirmation would be appreciated.

The code that is generating the assertion has been introduced by this patch: https://reviews.llvm.org/D80462
I can checkout that commit and see if the test case fails with it? Would that be a good experiment to do?

No need -- so long as we're all agreed that we haven't regressed anything between Clang 16 and Clang 17 here, that's all I'm really after.

[zahiraam]

In D152818#4456872, @aaron.ballman wrote:

In D152818#4456797, @zahiraam wrote:

In D152818#4456717, @aaron.ballman wrote:

In D152818#4456510, @zahiraam wrote:

In D152818#4456483, @nicolerabjohn wrote:

In D152818#4442116, @rjmccall wrote:

Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper?

It does not solve the problem, at least for my test case (linked in https://github.com/llvm/llvm-project/issues/63063) - we still hit the assertion.

Sorry! I will not be able to work on this until about September! I haven't tried to reproduce the issue with the test case in the link above.

We branch for the 17 release at the end of July, so I'm wondering whether there's anything we need to revert related to this? It looks like this assertion started firing in Clang 12.0.0 (https://cexplorer.testlabs.pro/z/Wh9o8W), so this doesn't seem to be a regression, but confirmation would be appreciated.

The code that is generating the assertion has been introduced by this patch: https://reviews.llvm.org/D80462
I can checkout that commit and see if the test case fails with it? Would that be a good experiment to do?

No need -- so long as we're all agreed that we haven't regressed anything between Clang 16 and Clang 17 here, that's all I'm really after.

I verified that it's failing from clang12.0.0 through clang 16.0.0. I think this confirms that's not a regression between clang 16 and clang 17. Doesn't it?

[aaron.ballman]

In D152818#4457013, @zahiraam wrote:

In D152818#4456872, @aaron.ballman wrote:

In D152818#4456797, @zahiraam wrote:

In D152818#4456717, @aaron.ballman wrote:

In D152818#4456510, @zahiraam wrote:

In D152818#4456483, @nicolerabjohn wrote:

In D152818#4442116, @rjmccall wrote:

Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper?

It does not solve the problem, at least for my test case (linked in https://github.com/llvm/llvm-project/issues/63063) - we still hit the assertion.

Sorry! I will not be able to work on this until about September! I haven't tried to reproduce the issue with the test case in the link above.

We branch for the 17 release at the end of July, so I'm wondering whether there's anything we need to revert related to this? It looks like this assertion started firing in Clang 12.0.0 (https://cexplorer.testlabs.pro/z/Wh9o8W), so this doesn't seem to be a regression, but confirmation would be appreciated.

The code that is generating the assertion has been introduced by this patch: https://reviews.llvm.org/D80462
I can checkout that commit and see if the test case fails with it? Would that be a good experiment to do?

No need -- so long as we're all agreed that we haven't regressed anything between Clang 16 and Clang 17 here, that's all I'm really after.

I verified that it's failing from clang12.0.0 through clang 16.0.0. I think this confirms that's not a regression between clang 16 and clang 17. Doesn't it?

Correct, it does, so nothing needs to be reverted.

clang/lib/CodeGen/CodeGenFunction.cpp

void CodeGenFunction::CGFPOptionsRAII::ConstructorHelper(FPOptions FPFeatures) {
   CGF.Builder.setDefaultConstrainedRounding(NewRoundingBehavior);
   auto NewExceptionBehavior =
       ToConstrainedExceptMD(static_cast<LangOptions::FPExceptionModeKind>(
           FPFeatures.getExceptionMode()));
   CGF.Builder.setDefaultConstrainedExcept(NewExceptionBehavior);
 
   CGF.SetFastMathFlags(FPFeatures);
 
   assert((CGF.CurFuncDecl == nullptr || CGF.Builder.getIsFPConstrained() ||
           isa<CXXConstructorDecl>(CGF.CurFuncDecl) ||
           isa<CXXDestructorDecl>(CGF.CurFuncDecl) ||
+          isa<FunctionDecl>(CGF.CurFuncDecl) ||
           (NewExceptionBehavior == llvm::fp::ebIgnore &&
            NewRoundingBehavior == llvm::RoundingMode::NearestTiesToEven)) &&
          "FPConstrained should be enabled on entire function");
 
   auto mergeFnAttrValue = [&](StringRef Name, bool Value) {
     auto OldValue =
         CGF.CurFn->getFnAttribute(Name).getValueAsBool();
     auto NewValue = OldValue & Value;

clang/test/CodeGen/pragma-fenv_access.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fcxx-exceptions \
+// RUN: -fexceptions -emit-llvm -o - %s | FileCheck %s
+
+// expected-no-diagnostics
+
+char b;
+template <bool, class c, class> using d = c;
+template <class, class> void e();
+template <class f> auto g(f k) { auto pt = *k; }
+class n {
+public:
+  n(char *);
+};
+template <class, class aa> using i = aa;
+template <class ab> constexpr bool j = true;
+struct o {
+  using l = char;
+  using ac = l *;
+};
+template <class m> class p {
+public:
+  m ad;
+};
+template <class> struct ae;
+template <class> class q {
+public:
+  using ac = o::ac;
+  void r() { g(af.h); }
+  struct {
+    ac h;
+  } af;
+};
+class s {
+  using ag = i<char, char>;
+  using ah = q<d<j<ag>, o, ae<char>>>;
+
+public:
+  s(char *);
+  char *t() { ai.ad.r(); }
+  p<ah> ai;
+};
+using aj = s;
+class ak : n {
+public:
+  using al = n;
+  ak(aj k) : al(k.t()) {}
+};
+class u : ak {
+protected:
+  u() : ak(0) {}
+};
+class am : u {};
+class v {
+public:
+  class w : am {
+  public:
+    w(aj) {}
+  };
+};
+class x : v {
+protected:
+  void y() { throw w(&b); }
+};
+class z : x {
+public:
+  z() { y(); }
+};
+#pragma STDC FENV_ACCESS ON
+void a() { z(); }
+
+// CHECK: define dso_local void @_Z1av()