[Clang] Reset FP options before function instantiations

[sepavloff]

Summary

Previously function template instantiations occurred with FP options
that were in effect at the end of translation unit. It was a problem
for late template parsing as these FP options were used as attributes of
AST nodes and may result in crash. To fix it FP options are set to the
state of the point of template definition.

[efriedma]

Using the FPOptions from the beginning of the file doesn't seem much better than the FPOptions at the end of the file. Don't we want to use the FPOptions from the point of definition of the template?

[sepavloff]

Use FPOptions from the place where template is defined rather than default

[sepavloff]

Ping.

[efriedma]

LGTM

[rjmccall]

clang/lib/Parse/ParseTemplate.cpp

1736 ↗

(On Diff #527800)

Ah, is this bug specific to the MSVC-compatible template logic?

[sepavloff]

clang/lib/Parse/ParseTemplate.cpp

1736 ↗

(On Diff #527800)

Exactly. This is MSVC bug-or-feature.

[rjmccall]

Hmm. Why are we clearing the FP pragma stack instead of saving the old context onto it and then restoring after instantiation? I don't think semantic analysis ever depends on enclosing members of the stack, does it?

Clearing the entire stack might not matter much if we're at the end of the translation unit, which is the normal time to instantiate things, but it would matter if we're eagerly instantiating within the translation unit, which we have to do for various reasons, including explicit instantiation and constexpr.

[sepavloff]

In D143241#4441812, @rjmccall wrote:

Hmm. Why are we clearing the FP pragma stack instead of saving the old context onto it and then restoring after instantiation? I don't think semantic analysis ever depends on enclosing members of the stack, does it?

It shouldn't but the function body may contain pragmas and in the case of errors we could have weird behavior. Anyway, parsing in this case occurs at the end of translation unit, so it should be safe to clear pragma stack.

Clearing the entire stack might not matter much if we're at the end of the translation unit, which is the normal time to instantiate things, but it would matter if we're eagerly instantiating within the translation unit, which we have to do for various reasons, including explicit instantiation and constexpr.

Explicit instantiation performs late parsing at the end of translation unit, just as implicit one. Constexpr functions are parsed as usualy, at the point of definition (see Sema::canDelayFunctionBody). It seems late parsing always occurs at the end of TU.

[Jake-Egan]

Hi, the following assertion is getting tripped on AIX:

Assertion failed: *FpPragmaCurrentValue == SemaObj->FpPragmaStack.DefaultValue && "Expected a default pragma float_control value", file  /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/clang/lib/Serialization/ASTReader.cpp, line 8150, void clang::ASTReader::UpdateSema()()

https://lab.llvm.org/buildbot/#/builders/214/builds/8309/steps/6/logs/FAIL__Clang__late-parsed-instantiations_cpp

[sepavloff]

In D143241#4475741, @Jake-Egan wrote:

Hi, the following assertion is getting tripped on AIX:

Assertion failed: *FpPragmaCurrentValue == SemaObj->FpPragmaStack.DefaultValue && "Expected a default pragma float_control value", file  /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/clang/lib/Serialization/ASTReader.cpp, line 8150, void clang::ASTReader::UpdateSema()()

https://lab.llvm.org/buildbot/#/builders/214/builds/8309/steps/6/logs/FAIL__Clang__late-parsed-instantiations_cpp

This crash is observed only on clang-ppc64-aix buildbot. I created an issue for it: https://github.com/llvm/llvm-project/issues/63704 and xfailed the test on this platform: https://reviews.llvm.org/rGd4a5673addd6 .

[thesamesam]

During our regular snapshot testing, we've hit this in Gentoo on 32-bit Linux x86: https://github.com/llvm/llvm-project/issues/63704#issuecomment-1631791518.

clang/include/clang/Sema/Sema.h

struct PragmaStack {
     }
 
     // Constructors.
     explicit PragmaStack(const ValueType &Default)
         : DefaultValue(Default), CurrentValue(Default) {}
 
     bool hasValue() const { return CurrentValue != DefaultValue; }
 
+    void clear() {
+      Stack.clear();
+      CurrentValue = DefaultValue;
+    }
+
     SmallVector<Slot, 2> Stack;
     ValueType DefaultValue; // Value used for PSK_Reset action.
     ValueType CurrentValue;
     SourceLocation CurrentPragmaLocation;
   };
   // FIXME: We should serialize / deserialize these if they occur in a PCH (but
   // we shouldn't do so if they're in a module).
 

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
   // queue of pending implicit instantiations that we will instantiate later,
   // while we're still within our own instantiation context.
   // This has to happen before LateTemplateParser below is called, so that
   // it marks vtables used in late parsed templates as used.
   GlobalEagerInstantiationScope GlobalInstantiations(*this,
                                                      /*Enabled=*/Recursive);
   LocalEagerInstantiationScope LocalInstantiations(*this);
 
+  // Reset FP option to the state specified by command line.
+  FpPragmaStack.clear();
+  FPFeaturesStateRAII SavedFPFeatures(*this);
+  CurFPFeatures = FPOptions(getLangOpts());
+
   // Call the LateTemplateParser callback if there is a need to late parse
   // a templated function definition.
   if (!Pattern && PatternDecl->isLateTemplateParsed() &&
       LateTemplateParser) {
     // FIXME: Optimize to allow individual templates to be deserialized.
     if (PatternDecl->isFromASTFile())
       ExternalSource->ReadLateParsedTemplates(LateParsedTemplateMap);
 

clang/test/CodeGen/fp-template.cpp

+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fdelayed-template-parsing -emit-llvm -o - %s | FileCheck %s
+
+template <typename Ty>
+Ty func_01(Ty x, Ty y) {
+  return x + y;
+}
+
+float func_02(float x, float y) {
+  return func_01(x, y);
+}
+
+// This pragma sets non-default rounding mode and then delayed parsing occurs.
+// Check that the parsing uses FP options defined by command line options and
+// not by this pragma.
+#pragma STDC FENV_ROUND FE_TOWARDZERO
+
+// CHECK-LABEL: define {{.*}} float @_Z7func_01IfET_S0_S0_
+// CHECK:       %add = fadd float %0, %1