1500640 - Multiple attributes are ignored by clang

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Steve Fink [:sfink] [:s:]]

In CheckedInt.h, there is the following constructor declaration:

    MOZ_IMPLICIT constexpr CheckedInt(U aValue) MOZ_NO_ARITHMETIC_EXPR_IN_ARGUMENT

this is problematic for GCC; it doesn't like the trailing MOZ_NO_ARITHMETIC_EXPR_IN_ARGUMENT. So I changed it to

    MOZ_IMPLICIT MOZ_NO_ARITHMETIC_EXPR_IN_ARGUMENT constexpr CheckedInt(U aValue)

This seems to work for GCC, but the clang plugin static analyzer now complains about having an implicit constructor, because it ignores the MOZ_IMPLICIT attribute.

Part of what is happening is that the matcher gets called twice: once with a decl (a clang::CXXConstructorDecl*) that seems to correspond to the constructor before specialization, which has the attributes; and once with the specialized constructor, which is missing the attributes.

I'm tracing through the clang code, and I don't see any sort of pointer being stored from the specialized to the unspecialized decl. :( It pulls various fields out of the original decl to create the specialized one. This is in clang::TemplateDeclInstantiator::VisitCXXMethodDecl in SemaTemplateInstantiateDecl.cpp.

You can reproduce this by compiling a file with the following code:

----

template<typename T>
class CheckedInt
{
public:
  __attribute__((annotate("moz_implicit")))
  __attribute__((annotate("other")))
  CheckedInt(int aValue)
  {
  }
};

CheckedInt<int> foo(7);

----

If I reorder the attributes, it "works" (as in, it doesn't emit the warning). I've stepped through that case, and *both* decls have the attribute then. I haven't traced through the logic of when it adds the attributes yet.

Comment 1

[Steve Fink [:sfink] [:s:]]

Uh... this seems like a "broken as designed" thing?

The problem is that you're only allowed a single AnnotateAttr. From AttrImpl.inc (in clang):

  auto *A = new (C) AnnotateAttr(getLocation(), C, getAnnotation(), getSpellingListIndex());

which is calling

  AnnotateAttr::AnnotateAttr(SourceRange R, ASTContext &Ctx, llvm::StringRef Annotation, unsigned SI)
    : InheritableParamAttr(attr::Annotate, R, SI, false, false)

where that base constructor is

  InheritableParamAttr::InheritableParamAttr(attr::Kind AK, SourceRange R, unsigned SpellingListIndex, bool IsLateParsed, bool DuplicatesAllowed)

note that DuplicatesAllowed=false.

It processes annotations in reverse order, so here it picks up __attribute__((annotate("other"))) successfully, then ignores moz_implicit because it already has an AnnotateAttr and duplicates are not allowed. :-(

I don't know if disallowing duplicates is somehow important to the desired semantics of annotate? If so, it seems like the best approach would be to define a new attribute in the plugin, but http://lists.llvm.org/pipermail/cfe-dev/2017-March/052937.html makes it sound like that's not possible quite yet. Alternatively, we could patch clang to add a new builtin attribute.

Comment 2

[Mike Hommey [:glandium]]

This was fixed in clang 7. See e.g. bug 1492743 comment 13. Bug 1492663 is set to upgrade us to clang 7, but is blocked on bug 1498072.

Comment 3

[Steve Fink [:sfink] [:s:]]

(We are now on clang 7.)