[PATCH] D11221: Fix crash when using __attribute__((section (".bss.name"))) incorrectly

[Aaron Ballman]

CCing in Richard who may have better ideas on the constant expression
code in this patch.

On Wed, Jul 15, 2015 at 11:12 AM, Jakub Kuderski <jakub.kuderski at arm.com> wrote:
> kuhar created this revision.
> kuhar added a subscriber: cfe-commits.
> kuhar set the repository for this revision to rL LLVM.
>
> 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;`
>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D11221
>
> Files:
>   include/clang/Basic/DiagnosticSemaKinds.td
>   lib/Sema/SemaDecl.cpp
>   lib/Sema/SemaDeclAttr.cpp
>   test/Sema/attr-section-bss.c

> Index: test/Sema/attr-section-bss.c
> ===================================================================
> --- /dev/null
> +++ test/Sema/attr-section-bss.c
> @@ -0,0 +1,24 @@
> +// RUN: %clang_cc1 -verify -fsyntax-only %s
> +
> +__attribute__((section(".bss.Name"))) int a = 3 * 5; // expected-error {{variable placed in BSS section (".bss.Name") must be zero-initialized, but 'a' is initialized with non-zero value}}
> +__attribute__((section(".bss.Name"))) int correct_a = 0 * 5;
> +
> +__attribute__((section(".bss.Name"))) float x = 3.0 * 0.997; // expected-error {{variable placed in BSS section (".bss.Name") must be zero-initialized, but 'x' is initialized with 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 (".bss.Sth")}}
> +
> +__attribute__((section(".bss.Name"))) int arr[] = {0, 2, 0}; // expected-error {{variable placed in BSS section (".bss.Name") must be zero-initialized, but 'arr' is initialized with 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 {{variable placed in BSS section (".bss") must be zero-initialized, but 'str' is initialized with 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 = {};
> Index: lib/Sema/SemaDeclAttr.cpp
> ===================================================================
> --- lib/Sema/SemaDeclAttr.cpp
> +++ lib/Sema/SemaDeclAttr.cpp
> @@ -2396,6 +2396,12 @@
>    SectionAttr *NewAttr = S.mergeSectionAttr(D, Attr.getRange(), Str, Index);
>    if (NewAttr)
>      D->addAttr(NewAttr);
> +
> +  if (isa<FunctionDecl>(D) && Str.startswith(".bss")) {
> +    S.Diag(Attr.getRange().getEnd(), diag::err_attribute_section_bss_function)
> +        << Str << NewAttr->getRange();

Should use Attr.getLoc() instead of Attr.getRange().getEnd().

> +    D->dropAttr<SectionAttr>();

I think this logic can simply preceed the mergeSectionAttr() call, in
which case you don't need to add and then drop the attribute.

> +  }
>  }
>
>  // Check for things we'd like to warn about, no errors or validation for now.
> Index: lib/Sema/SemaDecl.cpp
> ===================================================================
> --- lib/Sema/SemaDecl.cpp
> +++ lib/Sema/SemaDecl.cpp
> @@ -5302,6 +5302,103 @@
>    return false;
>  }
>
> +static bool isValueNonZero(APValue &Value) {

Can Value be made const?

> +  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, VarDecl &VD) {

Can VD be made const?

> +  APValue Result;
> +  SmallVector<PartialDiagnosticAt, 2> Notes;
> +  if (E->EvaluateAsInitializer(Result, S.Context, &VD, Notes))
> +    return isValueNonZero(Result);
> +
> +  if (auto *InitList = dyn_cast<InitListExpr>(E)) {

Should be const auto * (and trickle the explicit constness down).

> +    for (Stmt *Statement : *InitList) {
> +      if (auto *InitElt = dyn_cast<Expr>(Statement)) {
> +        if (InitElt->EvaluateAsInitializer(Result, S.Context, &VD, Notes)) {
> +          if (isExpressionNonZero(InitElt, S, VD))
> +            return true;
> +        } else if (isExpressionNonZero(InitElt, S, VD))
> +          return true;
> +      } else
> +        return true;
> +    }
> +    return false;
> +  }
> +  return true; // Expr E is neither evaluable initializer nor initializer list.
> +}

Richard, do we have a better way of doing this?

> +
> +static void checkAttributeSectionBSS(Sema &S, VarDecl &VD) {
> +  if (auto *SA = VD.getAttr<SectionAttr>()) {
> +    if (SA->getName().startswith(".bss")) {
> +      if (const Expr *InitExpr = VD.getInit()) {
> +        if (isExpressionNonZero(InitExpr, S, VD)) {
> +          S.Diag(SA->getRange().getEnd(),
> +                 diag::err_attribute_section_bss_nonzero)
> +              << SA->getName() << VD.getName() << SA->getRange()

No need to call getName() here for either use.

> +              << 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.
> @@ -5330,6 +5427,7 @@
>          S.Diag(Attr->getLocation(), diag::err_alias_is_definition) << VD;
>          VD->dropAttr<AliasAttr>();
>        }
> +      checkAttributeSectionBSS(S, *VD);
>      }
>    }
>
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td
> +++ include/clang/Basic/DiagnosticSemaKinds.td
> @@ -2176,6 +2176,11 @@
>    "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<
> +  "variable placed in BSS section (\"%0\") must be zero-initialized, but "
> +  "\'%1\' is initialized with non-zero value">;

When removing calls to getName(), you can also remove the manual
quoting from the diagnostic. The engine handles it properly for you
already. I think you should also drop the %0 from this diagnostic as
the name of the exact BSS section is unimportant to the user. (It sort
of implies that if you name the section differently, you can put
nonzero values into it).

> +def err_attribute_section_bss_function : Error<
> +  "functions cannot be placed in BSS section (\"%0\")">;

Same here as above.

>
>  def err_anonymous_property: Error<
>    "anonymous property is not supported">;
>

~Aaron