llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.02k stars 11.96k forks source link

-Wdangling-gsl does not work consistently with span across STL #109442

Closed marco-antognini-sonarsource closed 1 month ago

marco-antognini-sonarsource commented 1 month ago

I'm checking the behavior of Clang 19 vs 18 and I noticed that @hokein's https://github.com/llvm/llvm-project/pull/99622 introduces some nice warnings thanks to the added attributes on std::span.

However, it does not work with libc++.

Here is an example:

#include <span>
#include <string>

struct SpanAggr {
  std::span<char const> sv;
};

void test() {
  // No issue raised in 18.1
  // Issue raised from 19.1 thanks to https://github.com/llvm/llvm-project/pull/99622
  // but not with libc++
  SpanAggr a1{std::span<char const>{std::string{}}};
}

https://godbolt.org/z/Yfe78nsdb

IIUC, the problem boils down to the fact std::span is defined with a template specialization for dynamic extent and the ClassTemplatePartialSpecializationDecl does not get the PointerAttr:

namespace std {
inline constexpr size_t dynamic_extent = -1;
template <typename _Tp, size_t _Extent = dynamic_extent>
class span;

template <typename _Tp, size_t _Extent>
struct span {
  // Bla
};
template <typename _Tp>
struct span<_Tp, dynamic_extent> {
  // Bla
};
}  // namespace std

https://godbolt.org/z/nh1j4a7xE

marco-antognini-sonarsource commented 1 month ago

Now, I see at least two possible ways to solve this.

  1. We do something like this:
    diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
    index 112cf3d08182..a328da15d722 100644
    --- a/clang/lib/Sema/CheckExprLifetime.cpp
    +++ b/clang/lib/Sema/CheckExprLifetime.cpp
    @@ -381,7 +381,18 @@ static void handleGslAnnotatedTypes(IndirectLocalPath &Path, Expr *Call,
    if (auto *CCE = dyn_cast<CXXConstructExpr>(Call)) {
     const auto *Ctor = CCE->getConstructor();
     const CXXRecordDecl *RD = Ctor->getParent();
    -    if (CCE->getNumArgs() > 0 && RD->hasAttr<PointerAttr>())
    +    auto const hasPointerAttr = [&] {
    +      auto const *Record = RD;
    +      if (auto const *Spec =
    +              dyn_cast<ClassTemplateSpecializationDecl>(Record)) {
    +        Record = Spec->getSpecializedTemplate()
    +                     ->getTemplatedDecl()
    +                     ->getCanonicalDecl();
    +      }
    +
    +      return Record->hasAttr<PointerAttr>();
    +    };
    +    if (CCE->getNumArgs() > 0 && hasPointerAttr())
       VisitPointerArg(Ctor->getParamDecl(0), CCE->getArgs()[0], true);
    }
    }

i.e., look at the "main" decl for std::span here when we have a ClassTemplateSpecializationDecl.

But this is inaccurate for non-inferred types (addGslOwnerPointerAttributeIfNotExisting) as I imagine users could, theoretically, add an [[attribute]] in their code for some specialization but not others (although unlikely).

  1. Or we can patch the creation of specialization to behave like template declaration. https://github.com/llvm/llvm-project/blob/a9006bffa994d5afe9ad0b661b69d655658ab5e8/clang/lib/Sema/SemaTemplate.cpp#L2146 However, I’m not familiar enough with Sema to know how to do it cleanly.

Happy to hear people opinion and guidance.

steakhal commented 1 month ago

Tagging @hokein.

hokein commented 1 month ago

Thanks for the report and for catching this!

It seems to be a bug or perhaps an oversight in the implementation. The current implementation only updates the primary template declaration, but we should do the same thing for explicit template specializations as well.

marco-antognini-sonarsource commented 1 month ago

That was quick, thanks @hokein!