[LifetimeAnalysis] Add [[gsl::Pointer]] to llvm::StringRef

[mgehre]

Summary

This detected the bugs fixed in https://reviews.llvm.org/D66442 and https://reviews.llvm.org/D66440.

The warning itself was implemented in
https://reviews.llvm.org/D63954
https://reviews.llvm.org/D64256
https://reviews.llvm.org/D65120
https://reviews.llvm.org/D65127
https://reviews.llvm.org/D66152

[xazax.hun]

I do not know if there are some build bots with -Werror that would warn for unknown attributes. In case it fails the build bots we could extend the include/llvm/Support/Compiler.h header with a macro.

[mehdi_amini]

Seems like an attribute that could go on ArrayRef as well?

[mgehre]

Generally, ArrayRef is also a candidate. But there we have the complication that OwningArrayRef indirectly inherits from ArrayRef. One is a gsl::Owner, the other a gsl::Pointer, and we
need to make sure that we handle it correctly when OwningArrayRef is accessed through its base class which is then seen as a Pointer.
I would thus defer this part to a later PR.

[dexonsmith]

In D66443#1643395, @mgehre wrote:

Generally, ArrayRef is also a candidate. But there we have the complication that OwningArrayRef indirectly inherits from ArrayRef. One is a gsl::Owner, the other a gsl::Pointer, and we
need to make sure that we handle it correctly when OwningArrayRef is accessed through its base class which is then seen as a Pointer.
I would thus defer this part to a later PR.

Can OwningArrayRef be changed not to inherit from ArrayRef?

[bondhugula]

This revision leads to a pervasive warning from StringRef.h (on the release build with Clang/Clang++ 9.0.1):

llvm/include/llvm/ADT/StringRef.h:57:11: warning: unknown attribute 'Pointer' ignored [-Wunknown-attributes]

[mstorsjo]

In D66443#1986064, @bondhugula wrote:

This revision leads a pervasive warning from StringRef.h (on the release build with Clang/Clang++ 9.0.1):

llvm/include/llvm/ADT/StringRef.h:57:11: warning: unknown attribute 'Pointer' ignored [-Wunknown-attributes]

Same thing with GCC 7:

../include/llvm/ADT/StringRef.h:57:26: warning: ‘gsl::Pointer’ scoped attribute directive ignored [-Wattributes]
   class [[gsl::Pointer]] StringRef {
                          ^~~~~~~~~

[labath]

+1

builds with compilers which don't implement this attribute are basically useless. This should either be predicated on the compiler supporting this attribute, or -W(unknown-)attributes should be disabled in cmake.

[RKSimon]

In D66443#1986255, @labath wrote:

+1

builds with compilers which don't implement this attribute are basically useless. This should either be predicated on the compiler supporting this attribute, or -W(unknown-)attributes should be disabled in cmake.

Disabling -W(unknown-)attributes will just allow this kind of issue to become more rampant, this needs adding to llvm\support\compiler.h similar to how we manage LLVM_FALLTHROUGH

[mgehre]

I'll add it to Compiler.h

[mgehre]

PR is here: https://reviews.llvm.org/D78290

[vsk]

Hi @mgehre, thanks for working on this. I noticed that your work appeared on GitHub at remotes/llvm/StringRefGslPointer. I think those branches are just for releases. In the future, please stage work on your personal fork.

[mgehre]

Hi @vsk, sorry, I pushed that branch by accident to the wrong remote.

llvm/include/llvm/ADT/StringRef.h

namespace llvm {
 
   /// StringRef - Represent a constant reference to a string, i.e. a character
   /// array and a length, which need not be null terminated.
   ///
   /// This class does not own the string data, it is expected to be used in
   /// situations where the character data resides in some other buffer, whose
   /// lifetime extends past that of the StringRef. For this reason, it is not in
   /// general safe to store a StringRef.
-  class StringRef {
+  class [[gsl::Pointer]] StringRef {
   public:
     static const size_t npos = ~size_t(0);
 
     using iterator = const char *;
     using const_iterator = const char *;
     using size_type = size_t;
 
   private: