[cmake] Disable GCC 9's -Winit-list-lifetime warning in ArrayRef

[labath]

Summary

This is a new warning which fires when one stores a reference to the
initializer_list contents in a way which may outlive the
initializer_list which it came from. In llvm this warning is triggered
whenever someone uses the initializer_list ArrayRef constructor.

This is indeed a dangerous thing to do (I myself was bitten by that at
least once), but it is not more dangerous than calling other ArrayRef
constructors with temporary objects -- something which we are used to
and have accepted as a tradeoff for ArrayRef's efficiency.

Currently, this warnings generates so much output that it completely
obscures any actionable warnings, so this patch disables it.

[aaron.ballman]

Would it make sense to instead disable the warning more locally to ArrayRef's problematic constructor, rather than disabling it for the entire project? I can imagine the diagnostics on ArrayRef not being useful for us, but that doesn't mean we want to lose those diagnostics for all the other cases where we may not want to make the same tradeoff.

[rnk]

I think we can take this patch as-is on the basis that right now the GCC 9 build is full of warning spam. Fixing that doesn't need to be blocked on the warning cleanup or the careful addition of warning pragma push/pops and the associated ifdefs. I also suspect that the warning may affect the call site of the ArrayRef constructor, in which case the pragmas won't be localized, they will be everywhere. Please wait for @aaron.ballman to approve this, since I don't want to override his concern unilaterally.

[xbolva00]

I had wanted to suggest pragmas too but then I realized what basically rnk says:

I also suspect that the warning may affect the call site of the ArrayRef constructor, in which case the pragmas won't be localized, they will be everywhere.

So *probably* there is no “pragma” solution here..

[aaron.ballman]

In D70122#1749001, @xbolva00 wrote:

I had wanted to suggest pragmas too but then I realized what basically rnk says:

I also suspect that the warning may affect the call site of the ArrayRef constructor, in which case the pragmas won't be localized, they will be everywhere.

So *probably* there is no “pragma” solution here..

I agree that we don't want to add pragmas at all the use sites.

Maybe we could wrap the problematic constructor into static method of ArrayRef? Than, we could add warning push/pop pragma here. Could it work? And update all callsites to use this new static method..

This may be a possible solution worth looking into.

I'm accepting because I don't want to prevent -Werror builds with GCC 9 or requiring heroics, but I still think we want this diagnostic to be enabled in most cases and think it's unfortunate we'll lose its coverage.

[xbolva00]

Yeah, +1.

If pragma is not working in this case, ok, just land this patch.

[labath]

#pragma in the constructor is actually working in this case. The reason I did not implement that originally is because adding the pragma then requires adding more preprocessor cruft to to make sure that compiler which do *not* support this warning don't spam with the "unrecognised pragma" warnings. And I wasn't sure how much are we willing to go to accommodate some particular compiler. However, the consensus seems to be that this warning is worth it, so I'm going to rewrite this patch in a more targetted fashion.

I am running the build the new patch now, and so far I don't see any occurrences of this warning outside of ArrayRef.

[labath]

• disable the warning just in the ArrayRef constructor

[aaron.ballman]

LGTM!

[rnk]

llvm/include/llvm/ADT/ArrayRef.h

100

Use LLVM_GNUC_PREREQ to gracefully handle the case of __GNUC__ being undefined.

[rnk]

Looks good with the fix.

llvm/include/llvm/ADT/ArrayRef.h

110

ditto

llvm/include/llvm/ADT/ArrayRef.h

public:
     /*implicit*/ constexpr ArrayRef(const std::array<T, N> &Arr)
         : Data(Arr.data()), Length(N) {}
 
     /// Construct an ArrayRef from a C array.
     template <size_t N>
     /*implicit*/ constexpr ArrayRef(const T (&Arr)[N]) : Data(Arr), Length(N) {}
 
     /// Construct an ArrayRef from a std::initializer_list.
+#if LLVM_GNUC_PREREQ(9, 0, 0)
+// Disable gcc's warning in this constructor as it generates an enormous amount
+// of messages. Anyone using ArrayRef should already be aware of the fact that
+// it does not do lifetime extension.
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Winit-list-lifetime"
+#endif
     /*implicit*/ ArrayRef(const std::initializer_list<T> &Vec)
     : Data(Vec.begin() == Vec.end() ? (T*)nullptr : Vec.begin()),
       Length(Vec.size()) {}
+#if LLVM_GNUC_PREREQ(9, 0, 0)
+#pragma GCC diagnostic pop
+#endif
 
     /// Construct an ArrayRef<const T*> from ArrayRef<T*>. This uses SFINAE to
     /// ensure that only ArrayRefs of pointers can be converted.
     template <typename U>
     ArrayRef(
         const ArrayRef<U *> &A,
         typename std::enable_if<
            std::is_convertible<U *const *, T const *>::value>::type * = nullptr)