[MS-ABI] Allow #pragma section to choose for ZI data

[javed.absar]

Summary

This patch enables the msvc pragma section to choose whether zero initialized variables are to be placed in data_seg or bss_seg. The current
implementation ignores bss_seg directive for variables that are initialized (i.e. it does not check if the initialization is, in fact, to all zeros).
This patch now allows that. The variables are now placed in named bss_seg but only so If (a) the initialization is all zeros; AND (b) ''-falways-use-bss flag is set'. In other words, current functionality is not impacted and a useful feature is provided to users.

[rogfer01]

lib/Driver/Tools.cpp

5940

I think you mean "-falways-use-bss".

Please add a test that checks that the driver is forwarding to CC1 the right flag. You only have a CC1 test that cannot catch this bug.

lib/Sema/SemaDecl.cpp

10742–10743

This check wasn't in the original code. Can you explain why it is needed here?

If it is needed, maybe you want a test for this as well.

test/CodeGenCXX/zi-sections.cpp

39

Would it make sense a test that checks array fillers?

int ArrayWithFiller[10] = { 0 };
int ArrayWithFillerTwo[10] = { 0, 2 };
int ArrayWithAllFiller[10] = { };

[javed.absar]

Hi Roger:
Thanks for the review. I have extended the test and fixed driver flag passing that you pointed out.

Regarding the check in Sema::DeclarationIsZeroInitialized, the check if to keep the function return value consistent, in case function is called with a declaration that is without an initializer.
Best Regards,
Javed

[rogfer01]

Thanks @javed.absar

This LGTM now.

[thakis]

I'm surprised this is behind a flag. This pragma attempts to be compatible with cl.exe. Does cl.exe do this by default? If so, we should do it by default. If not, why add this as an incompatible thing to an MS extension instead of keying it off some attribute that works in non-MS mode too?

[javed.absar]

Hi Nico:
This is for the case when using '#pragma bss_seg..' feature outside of msvc context. That's the reason for the flag.
One could use attribute, but as attribute is attached to individual declaration, while pragma applies to whole file (unless superseded by another pragma), the latter is easier to use in some cases (i.e. when there are lots of such declarations). Hope this clarifies the question you asked.
Best Regards
Javed

[thakis]

Adding incompatible extensions to MS extensions sounds like bad precedent to me. I think it'd be good if a code owner could opine on this change before it goes in.

[rnk]

This functionality doesn't feel worth a driver flag. It customizes a very small aspect of a Microsoft extension that was only implemented for compatibility. It also reimplements the logic that LLVM uses in the backend to put global variables in .bss or .data. Can you elaborate on the actual use case? Is the goal to put all globals that would normally live in .bss into a custom section? If so, I think we should implement our own pragma to do that rather than piggybacking on the Microsoft one.

lib/AST/Expr.cpp

2841

I seriously doubt we need this much code to check if something is a zero. I'm sure we already have a way to do this.

[javed.absar]

Abandoning as there is a separate pragma clang section implementation now.

include/clang/AST/APValue.h

public:
   bool isComplexFloat() const { return Kind == ComplexFloat; }
   bool isLValue() const { return Kind == LValue; }
   bool isVector() const { return Kind == Vector; }
   bool isArray() const { return Kind == Array; }
   bool isStruct() const { return Kind == Struct; }
   bool isUnion() const { return Kind == Union; }
   bool isMemberPointer() const { return Kind == MemberPointer; }
   bool isAddrLabelDiff() const { return Kind == AddrLabelDiff; }
+  bool isAllZeros() const;
 
   void dump() const;
   void dump(raw_ostream &OS) const;
 
   void printPretty(raw_ostream &OS, ASTContext &Ctx, QualType Ty) const;
   std::string getAsString(ASTContext &Ctx, QualType Ty) const;
 
   APSInt &getInt() {

include/clang/AST/Expr.h

public:
 
   /// isConstantInitializer - Returns true if this expression can be emitted to
   /// IR as a constant, and thus can be used as a constant initializer in C.
   /// If this expression is not constant and Culprit is non-null,
   /// it is used to store the address of first non constant expr.
   bool isConstantInitializer(ASTContext &Ctx, bool ForRef,
                              const Expr **Culprit = nullptr) const;
 
+   /// isZeroInitializer - Returns true if this initializer is all zeros.
+   bool isZeroInitializer(ASTContext &Ctx, bool IsForRef,
+                          const Expr **Culprit = nullptr) const;
+
   /// EvalStatus is a struct with detailed info about an evaluation in progress.
   struct EvalStatus {
     /// \brief Whether the evaluated expression has side effects.
     /// For example, (f() && 0) can be folded, but it still has side effects.
     bool HasSideEffects;
 
     /// \brief Whether the evaluation hit undefined behavior.
     /// For example, 1.0 / 0.0 can be folded to Inf, but has undefined behavior.

include/clang/Basic/LangOptions.def

 
 BENIGN_LANGOPT(AccessControl     , 1, 1, "C++ access control")
 LANGOPT(CharIsSigned      , 1, 1, "signed char")
 LANGOPT(ShortWChar        , 1, 0, "unsigned short wchar_t")
 ENUM_LANGOPT(MSPointerToMemberRepresentationMethod, PragmaMSPointersToMembersKind, 2, PPTMK_BestCase, "member-pointer representation method")
 ENUM_LANGOPT(DefaultCallingConv, DefaultCallingConvention, 3, DCC_None, "default calling convention")
 
 LANGOPT(ShortEnums        , 1, 0, "short enum types")
+LANGOPT(AlwaysUseBSS, 1, 0, "place variables initialized to 0 in pragma bss_seg")
 
 LANGOPT(OpenCL            , 1, 0, "OpenCL")
 LANGOPT(OpenCLVersion     , 32, 0, "OpenCL version")
 LANGOPT(NativeHalfType    , 1, 0, "Native half type support")
 LANGOPT(NativeHalfArgsAndReturns, 1, 0, "Native half args and returns")
 LANGOPT(HalfArgsAndReturns, 1, 0, "half args and returns")
 LANGOPT(CUDA              , 1, 0, "CUDA")
 LANGOPT(OpenMP            , 32, 0, "OpenMP support and version of OpenMP (31, 40 or 45)")

include/clang/Driver/Options.td

def ffunction_sections : Flag<["-"], "ffunction-sections">, Group<f_Group>,
   HelpText<"Place each function in its own section (ELF Only)">;
 def fno_function_sections : Flag<["-"], "fno-function-sections">,
   Group<f_Group>, Flags<[CC1Option]>;
 def fdata_sections : Flag <["-"], "fdata-sections">, Group<f_Group>,
  Flags<[CC1Option]>, HelpText<"Place each data in its own section (ELF Only)">;
 def fno_data_sections : Flag <["-"], "fno-data-sections">, Group<f_Group>,
   Flags<[CC1Option]>;
 
+def falways_use_bss : Flag<["-"], "falways-use-bss">,Group<f_Group>,
+  Flags<[CC1Option]>,
+  HelpText<"Place variables initialized to 0 in pragma bss_seg">;
+def fno_always_use_bss : Flag<["-"], "fno-always-use-bss">,Group<f_Group>,
+  Flags<[CC1Option]>;
+
 def funique_section_names : Flag <["-"], "funique-section-names">,
   Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Use unique names for text and data sections (ELF Only)">;
 def fno_unique_section_names : Flag <["-"], "fno-unique-section-names">,
   Group<f_Group>, Flags<[CC1Option]>;
 
 def fstrict_return : Flag<["-"], "fstrict-return">, Group<f_Group>,
   Flags<[CC1Option]>,

include/clang/Sema/Sema.h

public:
   ActOnDecompositionDeclarator(Scope *S, Declarator &D,
                                MultiTemplateParamsArg TemplateParamLists);
   // Returns true if the variable declaration is a redeclaration
   bool CheckVariableDeclaration(VarDecl *NewVD, LookupResult &Previous);
   void CheckVariableDeclarationType(VarDecl *NewVD);
   bool DeduceVariableDeclarationType(VarDecl *VDecl, bool DirectInit,
                                      Expr *Init);
   void CheckCompleteVariableDeclaration(VarDecl *VD);
+  bool DeclarationIsZeroInitialized(VarDecl *VD);
   void CheckCompleteDecompositionDeclaration(DecompositionDecl *DD);
   void MaybeSuggestAddingStaticToDecl(const FunctionDecl *D);
 
   NamedDecl* ActOnFunctionDeclarator(Scope* S, Declarator& D, DeclContext* DC,
                                      TypeSourceInfo *TInfo,
                                      LookupResult &Previous,
                                      MultiTemplateParamsArg TemplateParamLists,
                                      bool &AddToScope);

lib/AST/APValue.cpp

void APValue::MakeMemberPointer(const ValueDecl *Member, bool IsDerivedMember,
   assert(isUninit() && "Bad state change");
   MemberPointerData *MPD = new ((void*)(char*)Data.buffer) MemberPointerData;
   Kind = MemberPointer;
   MPD->MemberAndIsDerivedMember.setPointer(Member);
   MPD->MemberAndIsDerivedMember.setInt(IsDerivedMember);
   MPD->resizePath(Path.size());
   memcpy(MPD->getPath(), Path.data(), Path.size()*sizeof(const CXXRecordDecl*));
 }
+
+bool APValue::isAllZeros() const {
+  switch (getKind()) {
+  case Uninitialized:
+    return false;
+  case Int:
+    return !getInt().getBoolValue();
+  case  Float:
+    return getFloat().isPosZero();
+  case ComplexInt:
+    return !getComplexIntReal().getBoolValue() && !getComplexIntImag().getBoolValue();
+  case ComplexFloat:
+    return getComplexFloatReal().isPosZero() && getComplexFloatImag().isPosZero();
+  default:
+    return false;
+  }
+  return false;
+}

lib/AST/Expr.cpp

bool Expr::isConstantInitializer(ASTContext &Ctx, bool IsForRef,
   // these, but they're common enough that we have to accept them.
   if (isEvaluatable(Ctx, SE_AllowUndefinedBehavior))
     return true;
   if (Culprit)
     *Culprit = this;
   return false;
 }
 
+/// Returns true if the initializer is all zeros.
+bool Expr::isZeroInitializer(ASTContext &Ctx, bool IsForRef,
+                             const Expr **Culprit) const {
+  if (IsForRef)
+    return false;
+
+  switch (getStmtClass()) {
+  default: break;
+  case CompoundLiteralExprClass: {
+    const Expr *Exp = cast<CompoundLiteralExpr>(this)->getInitializer();
+    return Exp->isZeroInitializer(Ctx, false, Culprit);
+  }
+  case DesignatedInitUpdateExprClass: {
+    const DesignatedInitUpdateExpr *DIUE = cast<DesignatedInitUpdateExpr>(this);
+    return DIUE->getBase()->isZeroInitializer(Ctx, false, Culprit) &&
+           DIUE->getUpdater()->isZeroInitializer(Ctx, false, Culprit);
+  }
+  case  InitListExprClass: {
+    const InitListExpr *ILE = cast<InitListExpr>(this);
+    // array initializer
+    if (ILE->getType()->isArrayType()) {
+      unsigned numInits = ILE->getNumInits();
+      for (unsigned i = 0; i < numInits; i++) {
+        if (!ILE->getInit(i)->isZeroInitializer(Ctx, false, Culprit))
+          return false;
+      }
+      return true;
+    }
+
+    // record initializer
+    if (ILE->getType()->isRecordType()) {
+      unsigned ElementNo = 0;
+      RecordDecl *RD = ILE->getType()->getAs<RecordType>()->getDecl();
+      for (const auto *Field : RD->fields()) {
+        // If this is a union, skip all the fields that aren't being initialized.
+        if (RD->isUnion() && ILE->getInitializedFieldInUnion() != Field)
+          continue;
+
+        // Don't emit anonymous bitfields, they just affect layout.
+        if (Field->isUnnamedBitfield())
+          continue;
+
+        if (ElementNo < ILE->getNumInits()) {
+          const Expr *Elt = ILE->getInit(ElementNo++);
+          if (Field->isBitField()) {
+            // Bitfields have to evaluate to an integer.
+            llvm::APSInt ResultTmp;
+            if (!Elt->EvaluateAsInt(ResultTmp, Ctx)) {
+              if (Culprit)
+                *Culprit = Elt;
+              return false;
+            }
+            if (ResultTmp != 0)
+              return false;
+          } else {
+            //bool RefType = Field->getType()->isReferenceType();
+            if (!Elt->isZeroInitializer(Ctx, false, Culprit))
+              return false;
+          }
+        }
+      }
+      return true;
+    }
+    break;
+  }
+  case ImplicitValueInitExprClass:
+  case NoInitExprClass:
+    return true;
+  case ParenExprClass:
+    return cast<ParenExpr>(this)->getSubExpr()
+      ->isZeroInitializer(Ctx, false, Culprit);
+  case GenericSelectionExprClass:
+    return cast<GenericSelectionExpr>(this)->getResultExpr()
+      ->isZeroInitializer(Ctx, false, Culprit);
+  case ChooseExprClass:
+    if (cast<ChooseExpr>(this)->isConditionDependent()) {
+      if (Culprit)
+        *Culprit = this;
+      return false;
+    }
+    return cast<ChooseExpr>(this)->getChosenSubExpr()
+      ->isZeroInitializer(Ctx, false, Culprit);
+  case UnaryOperatorClass: {
+    const UnaryOperator* Exp = cast<UnaryOperator>(this);
+    if (Exp->getOpcode() == UO_Extension)
+      return Exp->getSubExpr()->isZeroInitializer(Ctx, false, Culprit);
+    break;
+  }
+  case CXXFunctionalCastExprClass:
+  case CXXStaticCastExprClass:
+  case ImplicitCastExprClass:
+  case CStyleCastExprClass:
+  case ObjCBridgedCastExprClass:
+  case CXXDynamicCastExprClass:
+  case CXXReinterpretCastExprClass:
+  case CXXConstCastExprClass: {
+    const CastExpr *CE = cast<CastExpr>(this);
+
+    if (CE->getCastKind() == CK_NoOp ||
+        CE->getCastKind() == CK_LValueToRValue ||
+        CE->getCastKind() == CK_ToUnion ||
+        CE->getCastKind() == CK_ConstructorConversion ||
+        CE->getCastKind() == CK_NonAtomicToAtomic ||
+        CE->getCastKind() == CK_AtomicToNonAtomic ||
+        CE->getCastKind() == CK_IntToOCLSampler)
+      return CE->getSubExpr()->isZeroInitializer(Ctx, false, Culprit);
+
+    break;
+  }
+  case MaterializeTemporaryExprClass:
+    return cast<MaterializeTemporaryExpr>(this)->GetTemporaryExpr()
+      ->isZeroInitializer(Ctx, false, Culprit);
+
+  case SubstNonTypeTemplateParmExprClass:
+    return cast<SubstNonTypeTemplateParmExpr>(this)->getReplacement()
+      ->isZeroInitializer(Ctx, false, Culprit);
+  case CXXDefaultArgExprClass:
+    return cast<CXXDefaultArgExpr>(this)->getExpr()
+      ->isZeroInitializer(Ctx, false, Culprit);
+  case CXXDefaultInitExprClass:
+    return cast<CXXDefaultInitExpr>(this)->getExpr()
+      ->isZeroInitializer(Ctx, false, Culprit);
+  }
+
+  EvalResult Result;
+  if (EvaluateAsRValue(Result, Ctx))
+    return Result.Val.isAllZeros();
+  return false;
+}
+
 namespace {
   /// \brief Look for any side effects within a Stmt.
   class SideEffectFinder : public ConstEvaluatedExprVisitor<SideEffectFinder> {
     typedef ConstEvaluatedExprVisitor<SideEffectFinder> Inherited;
     const bool IncludePossibleEffects;
     bool HasSideEffects;
 
   public:

lib/Driver/Tools.cpp

if (!Args.hasFlag(
       KernelOrKext)
     CmdArgs.push_back("-fno-use-cxa-atexit");
 
   // -fms-extensions=0 is default.
   if (Args.hasFlag(options::OPT_fms_extensions, options::OPT_fno_ms_extensions,
                    IsWindowsMSVC))
     CmdArgs.push_back("-fms-extensions");
 
+  if (Args.hasFlag(options::OPT_falways_use_bss,
+                   options::OPT_fno_always_use_bss, false))
+    CmdArgs.push_back("-always-use-bss");
+
   // -fno-use-line-directives is default.
   if (Args.hasFlag(options::OPT_fuse_line_directives,
                    options::OPT_fno_use_line_directives, false))
     CmdArgs.push_back("-fuse-line-directives");
 
   // -fms-compatibility=0 is default.
   if (Args.hasFlag(options::OPT_fms_compatibility,
                    options::OPT_fno_ms_compatibility,

lib/Frontend/CompilerInvocation.cpp

if (const Arg *A = Args.getLastArg(OPT_fms_compatibility_version)) {
     if (VT.tryParse(A->getValue()))
       Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args)
                                                 << A->getValue();
     Opts.MSCompatibilityVersion = VT.getMajor() * 10000000 +
                                   VT.getMinor().getValueOr(0) * 100000 +
                                   VT.getSubminor().getValueOr(0);
   }
 
+  Opts.AlwaysUseBSS = Args.hasArg(OPT_falways_use_bss);
+
   // Mimicing gcc's behavior, trigraphs are only enabled if -trigraphs
   // is specified, or -std is set to a conforming mode.
   // Trigraphs are disabled by default in c++1z onwards.
   Opts.Trigraphs = !Opts.GNUMode && !Opts.MSVCCompat && !Opts.CPlusPlus1z;
   Opts.Trigraphs =
       Args.hasFlag(OPT_ftrigraphs, OPT_fno_trigraphs, Opts.Trigraphs);
 
   Opts.DollarIdents = Args.hasFlag(OPT_fdollars_in_identifiers,

lib/Sema/SemaDecl.cpp

                 EmptyAttrs, IdentLoc);
   Decl *Var = ActOnDeclarator(S, D);
   cast<VarDecl>(Var)->setCXXForRangeDecl(true);
   FinalizeDeclaration(Var);
   return ActOnDeclStmt(FinalizeDeclaratorGroup(S, DS, Var), IdentLoc,
                        AttrEnd.isValid() ? AttrEnd : IdentLoc);
 }
 
+/// Check if initialization is to zero
+bool Sema::DeclarationIsZeroInitialized(VarDecl *var) {
+  Expr* InitExpr = var->getInit();
+  if (!InitExpr)
+    return var->hasGlobalStorage();
+
+  const Expr *Culprit;
+  if (!InitExpr->isConstantInitializer(Context, false, &Culprit))
+    return false;
+  return InitExpr->isZeroInitializer(Context, false, &Culprit);
+}
+
 void Sema::CheckCompleteVariableDeclaration(VarDecl *var) {
   if (var->isInvalidDecl()) return;
 
   if (getLangOpts().OpenCL) {
     // OpenCL v2.0 s6.12.5 - Every block variable declaration must have an
     // initialiser
     if (var->getTypeSourceInfo()->getType()->isBlockPointerType() &&
         !var->hasInit()) {

   // Apply section attributes and pragmas to global variables.
   bool GlobalStorage = var->hasGlobalStorage();
   if (GlobalStorage && var->isThisDeclarationADefinition() &&
       ActiveTemplateInstantiations.empty()) {
     PragmaStack<StringLiteral *> *Stack = nullptr;
     int SectionFlags = ASTContext::PSF_Implicit | ASTContext::PSF_Read;
     if (var->getType().isConstQualified())
       Stack = &ConstSegStack;
-    else if (!var->getInit()) {
+    else if (!var->getInit() ||
+             (getLangOpts().AlwaysUseBSS &&
+              DeclarationIsZeroInitialized(var))) {
       Stack = &BSSSegStack;
       SectionFlags |= ASTContext::PSF_Write;
     } else {
       Stack = &DataSegStack;
       SectionFlags |= ASTContext::PSF_Write;
     }
     if (Stack->CurrentValue && !var->hasAttr<SectionAttr>()) {
       var->addAttr(SectionAttr::CreateImplicit(

test/CodeGenCXX/zi-sections.cpp

+// RUN: %clang_cc1 -emit-llvm -triple i686-pc-win32 -fms-extensions -o - %s | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-ZI-DATA
+// RUN: %clang_cc1 -emit-llvm -triple i686-pc-win32 -fms-extensions -falways-use-bss -o - %s | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-ZI-BSS
+// Test that when '-falways-use-bss' is specified, variables initialized with zero are assigned to bss_seg.
+#pragma bss_seg(".bss.1")
+#pragma data_seg(".data.1")
+extern "C" {
+short ShortZero = 0;
+short ShortTwo = 2;
+short ShortMinusTwo = -2;
+short ShortUnInit;
+
+int IntZero = 0;
+int IntTwo = 2;
+int IntMinusTwo = -2;
+int IntUnInit;
+
+float FloatZero = 0.0;
+float FloatMinusZero = -0.0;
+float FloatTwo = 2.0;
+float FloatUnInit;
+
+double DoubleZero = 0.0;
+double DoubleMinusZero = -0.0;
+double DoubleTwo = 2.0;
+double DoubleUnInit;
+
+struct S {
+  int x, y;
+  int z;
+};
+struct S StructZero = { 0, 0, 0 };
+struct S StructTwo = { 0, 0, 2};
+struct S StructMinusTwo = {0, -2, 0};
+struct S StructUnInit;
+
+int ArrayZero[] = {0, 0, 0};
+int ArrayTwo[] = {0, 2, 0};
+int ArrayMinusTwo[] = {0, -2, 0};
+int ArrayUnInit;
+// CHECK-ZI-DATA: @ShortZero = global i16 0, section ".data.1", align 2
+// CHECK-ZI-BSS: @ShortZero = global i16 0, section ".bss.1", align 2
+// CHECK: @ShortTwo = global i16 2, section ".data.1", align 2
+// CHECK: @ShortMinusTwo = global i16 -2, section ".data.1", align 2
+// CHECK: @ShortUnInit = global i16 0, section ".bss.1", align 2
+//
+// CHECK-ZI-DATA: @IntZero = global i32 0, section ".data.1", align 4
+// CHECK-ZI-BSS: @IntZero = global i32 0, section ".bss.1", align 4
+// CHECK: @IntTwo = global i32 2, section ".data.1", align 4
+// CHECK: @IntMinusTwo = global i32 -2, section ".data.1", align 4
+// CHECK: @IntUnInit = global i32 0, section ".bss.1", align 4
+//
+// CHECK-ZI-DATA: @FloatZero = global float 0.000000e+00, section ".data.1", align 4
+// CHECK-ZI-BSS: @FloatZero = global float 0.000000e+00, section ".bss.1", align 4
+// CHECK: @FloatMinusZero = global float -0.000000e+00, section ".data.1", align 4
+// CHECK: @FloatTwo = global float 2.000000e+00, section ".data.1", align 4
+// CHECK: @FloatUnInit = global float 0.000000e+00, section ".bss.1", align 4
+//
+// CHECK-ZI-DATA: @DoubleZero = global double 0.000000e+00, section ".data.1", align 8
+// CHECK-ZI-BSS: @DoubleZero = global double 0.000000e+00, section ".bss.1", align 8
+// CHECK: @DoubleMinusZero = global double -0.000000e+00, section ".data.1", align 8
+// CHECK: @DoubleTwo = global double 2.000000e+00, section ".data.1", align 8
+// CHECK: @DoubleUnInit = global double 0.000000e+00, section ".bss.1", align 8
+//
+// CHECK-ZI-DATA: @StructZero = global %struct.S zeroinitializer, section ".data.1", align 4
+// CHECK-ZI-BSS: @StructZero = global %struct.S zeroinitializer, section ".bss.1", align 4
+// CHECK: @StructTwo = global %struct.S { i32 0, i32 0, i32 2 }, section ".data.1", align 4
+// CHECK: @StructMinusTwo = global %struct.S { i32 0, i32 -2, i32 0 }, section ".data.1", align 4
+// CHECK: @StructUnInit = global %struct.S zeroinitializer, section ".data.1", align 4
+//
+// CHECK-ZI-DATA: @ArrayZero = global [3 x i32] zeroinitializer, section ".data.1", align 4
+// CHECK-ZI-BSS: @ArrayZero = global [3 x i32] zeroinitializer, section ".bss.1", align 4
+// CHECK: @ArrayTwo = global [3 x i32] [i32 0, i32 2, i32 0], section ".data.1", align 4
+// CHECK: @ArrayMinusTwo = global [3 x i32] [i32 0, i32 -2, i32 0], section ".data.1", align 4
+// CHECK: @ArrayUnInit = global i32 0, section ".bss.1", align 4
+}