Fix crash when using __attribute__((section (".bss.name"))) incorrectly

[kuhar]

Summary

Previously, using attribute((section("bss.name")) incorrectly caused clang to crash displaying error from backed. This patch makes clang's frontend produce clear diagnostic/error.

Two main cases that are handled is using the attribute with functions and with variables initialized to non-zero values.
__attribute__((section(".bss.name"))) void func() { }
__attribute__((section(".bss.name"))) int a = 3;

[rsmith]

High-level comments:

1. This is wrong for C++. A non-zero initializer there is not a problem unless we are required to perform constant initialization for the variable. In any other case, we should emit the variable to the specified section and emit a dynamic initializer for it. (We'll need to check that globalopt doesn't optimize away that initialization and give the variable a non-zero initializer.)
2. It seems unnecessarily kind to allow any initializer at all on these variables. If we want to reject these cases, why not just reject any case where the variable has a specified initializer? (I think it's less reasonable to do this in C++, but whatever we do, making the constant evaluator smarter should not result in us rejecting more code.)
3. Sema is not in a position to decide for itself which constants / types will correspond to all-zero-bits (and for instance the MS ABI and Itanium ABI will disagree on whether some member pointer types are all-zero-bits). It should ask the CXXABI object to determine that on its behalf.
4. We should not be hard-coding knowledge of a target-specific ".bss" section into Sema. We should determine if the section has known properties by asking the target.

include/clang/Basic/DiagnosticSemaKinds.td

2180–2183

No need for \ before ' here.

[kuhar]

Thanks for review.
Aaron - fixed things you pointed out, except not dropping attribute - I'm not entirely convinced, because attributes are being dropped on error in the entire file...

Richard:
ad. 1. This should be fixed now - I only report error when variables are initialized with some 'constant expression', so that dynamic initializers are allowed.
ad. 2.

I think it's less reasonable to do this in C++>

Could you explain why?
ad. 3. I didn't know what to do with member pointers, so I left them unchecked. Is there a better way to determine non-zeroness of variable? Or maybe I should add it somewhere, so it can be target-specific (CXXABI.h?)?
ad. 4. Are there some existing mechanisms to do so, or could explain how it should be done? I've only found a function for validating section names (isValidSectionSpecifier in TargetInfo.h).
I would really appreciate your further help on this.

[joerg]

Why should this be in the frontend? Arguable, writing assembler with this is not wrong and if anything, the diagnostic should really be in the MC layer. I don't think precision of diagnostic is really that high a priority here, which would be the normal concern with errors from the MC layer. I don't think this is different from declaring multiple symbols of the same name etc.

[kuhar]

In D11221#207708, @joerg wrote:

Why should this be in the frontend? Arguable, writing assembler with this is not wrong and if anything, the diagnostic should really be in the MC layer. I don't think precision of diagnostic is really that high a priority here, which would be the normal concern with errors from the MC layer. I don't think this is different from declaring multiple symbols of the same name etc.

I'm a Clang newbie and I'm not really sure which approach is the best.
Richard, what do you think about this? I would really appreciate your opinion and some help.

[kuhar]

ping! Richard, I'm not sure what to do here. Do you have any input?

include/clang/Basic/DiagnosticSemaKinds.td

 
 def err_only_annotate_after_access_spec : Error<
   "access specifier can only have annotation attributes">;
 
 def err_attribute_section_invalid_for_target : Error<
   "argument to 'section' attribute is not valid for this target: %0">;
 def warn_mismatched_section : Warning<
   "section does not match previous declaration">, InGroup<Section>;
+def err_attribute_section_bss_nonzero : Error<
+  "variables placed in BSS section must be zero-initialized, but "
+  "%0 is initialized with a non-zero value">;
+def err_attribute_section_bss_function : Error<
+  "functions cannot be placed in BSS section">;
 
 def err_anonymous_property: Error<
   "anonymous property is not supported">;
 def err_property_is_variably_modified : Error<
   "property %0 has a variably modified type">;
 def err_no_accessor_for_property : Error<
   "no %select{getter|setter}0 defined for property %1">;
 def error_cannot_find_suitable_accessor : Error<

lib/Sema/SemaDecl.cpp

if (lifetime && lifetime != Qualifiers::OCL_ExplicitNone &&
         << var->getType();
       return true;
     }
   }
   
   return false;
 }
 
+static bool isValueNonZero(const APValue &Value) {
+  switch (Value.getKind()) {
+  case APValue::Uninitialized:
+    return false;
+  case APValue::Int:
+    return Value.getInt() != 0;
+  case APValue::Float:
+    return Value.getFloat().isNonZero();
+  case APValue::ComplexInt: {
+    bool isRealNonZero = Value.getComplexIntReal() != 0;
+    bool isImagNonZero = Value.getComplexIntImag() != 0;
+    return isRealNonZero || isImagNonZero;
+  }
+  case APValue::ComplexFloat: {
+    bool isRealNonZero = Value.getComplexFloatReal().isNonZero();
+    bool isImagNonZero = Value.getComplexFloatImag().isNonZero();
+    return isRealNonZero || isImagNonZero;
+  }
+  case APValue::Vector: {
+    for (unsigned i = 0, e = Value.getVectorLength(); i < e; ++i) {
+      if (isValueNonZero(Value.getVectorElt(i)))
+        return true;
+    }
+    return false;
+  }
+  case APValue::Array: {
+    for (unsigned i = 0, e = Value.getArrayInitializedElts(); i < e; ++i) {
+      if (isValueNonZero(Value.getArrayInitializedElt(i)))
+        return true;
+    }
+    return false;
+  }
+  case APValue::Struct: {
+    for (unsigned i = 0, e = Value.getStructNumBases(); i < e; ++i) {
+      if (isValueNonZero(Value.getStructBase(i)))
+        return true;
+    }
+    for (unsigned i = 0, e = Value.getStructNumFields(); i < e; ++i) {
+      if (isValueNonZero(Value.getStructField(i)))
+        return true;
+    }
+    return false;
+  }
+  case APValue::Union:
+    return isValueNonZero(Value.getUnionValue());
+
+  // For other kinds we cannot tell and assume non-zero.
+  case APValue::LValue:
+    return false;
+  case APValue::MemberPointer:
+    return false;
+  case APValue::AddrLabelDiff:
+    return false;
+  }
+
+  llvm_unreachable("unhandled APValue kind");
+}
+
+static bool isExpressionNonZero(const Expr *E, Sema &S, const VarDecl &VD) {
+  APValue Result;
+  SmallVector<PartialDiagnosticAt, 2> Notes;
+  if (E->EvaluateAsInitializer(Result, S.Context, &VD, Notes))
+    return isValueNonZero(Result);
+
+  if (const auto *InitList = dyn_cast<InitListExpr>(E)) {
+    for (Stmt *Statement : *InitList) {
+      if (const auto *InitElt = dyn_cast<Expr>(Statement)) {
+          if (isExpressionNonZero(InitElt, S, VD))
+            return true;
+      } else
+        return false;
+    }
+    return false;
+  }
+  return false; // Expr E is neither evaluable initializer nor initializer list.
+}
+
+static void checkAttributeSectionBSS(Sema &S, VarDecl &VD) {
+  if (const auto *SA = VD.getAttr<SectionAttr>()) {
+    if (SA->getName().startswith(".bss")) {
+      if (const Expr *InitExpr = VD.getInit()) {
+        if (isExpressionNonZero(InitExpr, S, VD)) {
+          S.Diag(SA->getLocation(),
+                 diag::err_attribute_section_bss_nonzero)
+              << &VD << SA->getRange() << InitExpr->getSourceRange();
+          
+          VD.dropAttr<SectionAttr>();
+        }
+      }
+    }
+  }
+}
+
 static void checkAttributesAfterMerging(Sema &S, NamedDecl &ND) {
   // Ensure that an auto decl is deduced otherwise the checks below might cache
   // the wrong linkage.
   assert(S.ParsingInitForAutoVars.count(&ND) == 0);
 
   // 'weak' only applies to declarations with external linkage.
   if (WeakAttr *Attr = ND.getAttr<WeakAttr>()) {
     if (!ND.isExternallyVisible()) {
static void checkAttributesAfterMerging(Sema &S, NamedDecl &ND) {
   if (auto *VD = dyn_cast<VarDecl>(&ND)) {
     if (VD->hasInit()) {
       if (const auto *Attr = VD->getAttr<AliasAttr>()) {
         assert(VD->isThisDeclarationADefinition() &&
                !VD->isExternallyVisible() && "Broken AliasAttr handled late!");
         S.Diag(Attr->getLocation(), diag::err_alias_is_definition) << VD;
         VD->dropAttr<AliasAttr>();
       }
+      checkAttributeSectionBSS(S, *VD);
     }
   }
 
   // 'selectany' only applies to externally visible variable declarations.
   // It does not apply to functions.
   if (SelectAnyAttr *Attr = ND.getAttr<SelectAnyAttr>()) {
     if (isa<FunctionDecl>(ND) || !ND.isExternallyVisible()) {
       S.Diag(Attr->getLocation(),

lib/Sema/SemaDeclAttr.cpp

if (!Error.empty()) {
     << Error;
     return;
   }
 
   unsigned Index = Attr.getAttributeSpellingListIndex();
   SectionAttr *NewAttr = S.mergeSectionAttr(D, Attr.getRange(), Str, Index);
   if (NewAttr)
     D->addAttr(NewAttr);
+
+  if (isa<FunctionDecl>(D) && Str.startswith(".bss")) {
+    S.Diag(Attr.getLoc(), diag::err_attribute_section_bss_function)
+       << NewAttr->getRange();
+    D->dropAttr<SectionAttr>();
+  }
 }
 
 // Check for things we'd like to warn about, no errors or validation for now.
 // TODO: Validation should use a backend target library that specifies
 // the allowable subtarget features and cpus. We could use something like a
 // TargetCodeGenInfo hook here to do validation.
 void Sema::checkTargetAttr(SourceLocation LiteralLoc, StringRef AttrStr) {
   for (auto Str : {"tune=", "fpmath="})

test/Sema/attr-section-bss.c

+// RUN: %clang_cc1 -verify -fsyntax-only %s
+
+__attribute__((section(".bss.Name"))) int a = 3 * 5; // expected-error {{variables placed in BSS section must be zero-initialized, but 'a' is initialized with a non-zero value}}
+__attribute__((section(".bss.Name"))) int correct_a = 0 * 5;
+
+__attribute__((section(".bss.Name"))) float x = 3 * 0.997; // expected-error {{variables placed in BSS section must be zero-initialized, but 'x' is initialized with a non-zero value}}
+__attribute__((section(".bss.Name"))) float correct_x = 0.0 * 0.997;
+
+__attribute__((section(".bss.Sth"))) void foo() {} // expected-error {{functions cannot be placed in BSS section}}
+
+__attribute__((section(".bss.Name"))) int arr[] = {0, 2, 0}; // expected-error {{variables placed in BSS section must be zero-initialized, but 'arr' is initialized with a non-zero value}}
+__attribute__((section(".bss.Name"))) int correct_arr[] = {0, 0, 0};
+
+typedef struct {
+  float a;
+  int b;
+  double arr[3];
+} TestStruct;
+
+__attribute__((section(".bss"))) TestStruct str = {0.0, 0, {0.0, 1.0, 0.0}}; // expected-error {{variables placed in BSS section must be zero-initialized, but 'str' is initialized with a non-zero value}}}
+__attribute__((section(".bss"))) TestStruct correct_str = {0.0, 0, {0.0, 0.0, 0.0}};
+
+typedef struct {} EmptyStruct;
+__attribute__((section(".bss.Name"))) EmptyStruct estr = {};
+