New warnings when building trunk with GCC 9

Hello,

GCC 9.0 introduces a new warning checkers and some of them found possible issues in LLVM.

In file included from /home/davidbolvansky/trunk/llvm/include/llvm/Analysis/LazyCallGraph.h:38,
from /home/davidbolvansky/trunk/llvm/unittests/Analysis/LazyCallGraphTest.cpp:10:
/home/davidbolvansky/trunk/llvm/include/llvm/ADT/ArrayRef.h: In instantiation of ‘llvm::ArrayRef::ArrayRef(const std::initializer_list<_Tp>&) [with T = llvm::LazyCallGraph::Node*]’:
/home/davidbolvansky/trunk/llvm/unittests/Analysis/LazyCallGraphTest.cpp:1169:52: required from here
/home/davidbolvansky/trunk/llvm/include/llvm/ADT/ArrayRef.h:102:37: warning: initializing ‘llvm::ArrayRefllvm::LazyCallGraph::Node*::Data’ from ‘std::initializer_listllvm::LazyCallGraph::Node*::begin’ does not extend the lifetime of the underlying array [-Winit-list-lifetime]

In file included from /home/davidbolvansky/trunk/llvm/unittests/ADT/SmallVectorTest.cpp:14:
/home/davidbolvansky/trunk/llvm/include/llvm/ADT/SmallVector.h: In member function ‘virtual void {anonymous}::SmallVectorTest_InitializerList_Test::TestBody()’:
/home/davidbolvansky/trunk/llvm/include/llvm/ADT/SmallVector.h:502:7: warning: array subscript 1 is outside array bounds of ‘int [1]’ [-Warray-bounds]
502 | ++EltPtr;

/home/davidbolvansky/trunk/llvm/unittests/ADT/SmallVectorTest.cpp:994:30: note: while referencing ‘’
994 | V2.insert(V2.begin() + 1, 5);

/home/davidbolvansky/trunk/llvm/unittests/ExecutionEngine/Orc/CompileOnDemandLayerTest.cpp:79:40: required from here
/home/davidbolvansky/trunk/llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h:314:29: warning: redundant move in return statement [-Wredundant-move]
314 | return std::move(Err);

In file included from /home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-spec-builders.h:75,
from /home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-function-mockers.h:43,
from /home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock.h:61,
from /home/davidbolvansky/trunk/llvm/include/llvm/Testing/Support/Error.h:17,
from /home/davidbolvansky/trunk/llvm/unittests/DebugInfo/MSF/MappedBlockStreamTest.cpp:15:
/home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-matchers.h: In instantiation of ‘testing::internal::PredicateFormatterFromMatcher::PredicateFormatterFromMatcher(M) [with M = llvm::FailedMatcher]’:
/home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-matchers.h:1880:10: required from ‘testing::internal::PredicateFormatterFromMatcher testing::internal::MakePredicateFormatterFromMatcher(M) [with M = llvm::FailedMatcher]’
/home/davidbolvansky/trunk/llvm/unittests/DebugInfo/MSF/MappedBlockStreamTest.cpp:96:3: required from here
/home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-matchers.h:1836:75: warning: implicitly-declared ‘constexpr llvm::FailedMatcher::FailedMatcher(const llvm::FailedMatcher&)’ is deprecated [-Wdeprecated-copy]
1836 | explicit PredicateFormatterFromMatcher(M m) : matcher_(internal::move(m)) {}

^
In file included from /home/davidbolvansky/trunk/llvm/utils/unittest/googletest/include/gtest/gtest-printers.h:103,
from /home/davidbolvansky/trunk/llvm/include/llvm/Testing/Support/SupportHelpers.h:16,
from /home/davidbolvansky/trunk/llvm/include/llvm/Testing/Support/Error.h:15,
from /home/davidbolvansky/trunk/llvm/unittests/DebugInfo/MSF/MappedBlockStreamTest.cpp:15:
/home/davidbolvansky/trunk/llvm/utils/unittest/googletest/include/gtest/internal/gtest-port.h:873:8: note: because ‘llvm::FailedMatcher’ has user-provided ‘void llvm::FailedMatcher::operator=(const llvm::FailedMatcher&)’
873 | void operator=(type const &)
^~~~~~~~
/home/davidbolvansky/trunk/llvm/utils/unittest/googlemock/include/gmock/gmock-generated-matchers.h:1417:5: note: in expansion of macro ‘GTEST_DISALLOW_ASSIGN_’
1417 | GTEST_DISALLOW_ASSIGN_(name##Matcher);
^~~~~~~~~~~~~~~~~~~~~~
/home/davidbolvansky/trunk/llvm/include/llvm/Testing/Support/Error.h:145:1: note: in expansion of macro ‘MATCHER’
145 | MATCHER(Failed, “”) { return !arg.Success(); }

Great to see GCC catching the deprecated ctor case - would be great to get that implemented in Clang at some point (& enabled for LLVM builds). (rtrieu@)
The vector one looks like a false positive? You’re allowed to form a pointer to an element one past the end of a singular object but it looks like that’s what GCC’s warning on.
Initializer list - maybe a false positive too, given ArrayRef is intended to refer to a temporary object - I think the initializer_list’s lifetime is to teh end of the full expression where it was written, so this can still be OK.
The ORC/redundant-move one looks correct - I wonder why Clang didn’t diagnose that (rtrieu@)?

Note that the move (into the implicit JITSymbol(Error) ctor) is only redundant if the compiler implements a fix for <http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1579> "Return by converting move constructor". (But not sure whether the LLVM compiler baselines imply that, anyway. In LibreOffice it forced me to introduce ugly #ifs, to not have to disable that warning outright, <https://cgit.freedesktop.org/libreoffice/core/commit/?id=dc06c8f4989fc28d0c31ebd333e53dfe0e0f5f66> "-Werror=redundant-move (GCC 9), take two".)

/home/davidbolvansky/trunk/llvm/unittests/ExecutionEngine/Orc/CompileOnDemandLayerTest.cpp:79:40:
required from here
/home/davidbolvansky/trunk/llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h:314:29:
warning: redundant move in return statement [-Wredundant-move]
314 | return std::move(Err);

Note that the move (into the implicit JITSymbol(Error) ctor) is only
redundant if the compiler implements a fix for
<http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1579>
“Return by converting move constructor”. (But not sure whether the LLVM
compiler baselines imply that, anyway.

Looks like that was accepted for C++14, not 11. So I don’t believe it’s valid to remove the std::move in C++11 code. (& I believe Clang’s warning correctly diagnoses this only in the right language versions) - so we probably want to disable the buggy GCC warning here, then.

There is a new discussion related to -Wredundant-move warning on GCC bugzilla.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87300

pi 14. 9. 2018 o 9:53 David Blaikie <dblaikie@gmail.com> napísal(a):

Fair point made on that thread - that this is a DR, so technically the std::move is pessimizing even in C++11 mode. Richard: Any thoughts on generalizing the warning to cover these cases even in C++11 mode?

Yes, we should produce this warning in C++11 mode too. (I could be misrecalling, but I think the rationale for the current behaviour is based on historical GCC behaviour.)

I’ll look into the pessimizing move warning, then after that, the deprecated copy warning.

  • Erik, who implemented DR1579

Originally, I had the warning similar to GCC’s warning, but took it out due to not having DR1579 implemented in clang (warning changed in r243594)

Erik in r274291 implemented DR1579, although PR27785 didn’t mention anything about std::move

It looks like what’s happening is that Clang and GCC handles the return differently. Clang needs the std::move call to use the move constructor while GCC will use the move constructor with or without the std::move call. This means that the warning is currently correct when running on either compiler.

This is a reduced example. Compiled with Clang, it will print “move constructor” then “copy constructor”. GCC will print “move constructor” twice.

#include
#include

struct A {
A(int) {}
A() { std::cout << “empty constructor”; }
A(const A&) { std::cout << “copy constructor\n”; }
A(A&&) { std::cout << “move constructor\n”; }
};
struct B {
B(A a) {}
};

A getA() { return A(1); }

B run1() {
A a = getA();
return std::move(a);
}

B run2() {
A a = getA();
return a;
}

int main() {
std::cout << “with move:\n”;
run1();
std::cout << “no move:\n”;
run2();
}

  • Erik, who implemented DR1579

Originally, I had the warning similar to GCC’s warning, but took it out due to not having DR1579 implemented in clang (warning changed in r243594)

Erik in r274291 implemented DR1579, although PR27785 didn’t mention anything about std::move

It looks like what’s happening is that Clang and GCC handles the return differently. Clang needs the std::move call to use the move constructor while GCC will use the move constructor with or without the std::move call. This means that the warning is currently correct when running on either compiler.

This is a reduced example. Compiled with Clang, it will print “move constructor” then “copy constructor”. GCC will print “move constructor” twice.

GCC gets the rule “wrong”. The rule in question (http://eel.is/c++draft/class.copy.elision#3.sentence-2) only applies when the selected B constructor takes A&& as its parameter type.

But the rule gets the rule wrong too – EWG wants that condition removed, so that it’s never necessary to write
return std::move(local_var);

I think the rule was written as it is to avoid breaking (obscure) working code, but before we had sufficient experience with rvalue references to understand how reasonable it is to pass by value in such cases. The time has come to simplify it.

I have a patch in progress, but lacked the time to finish updating test cases etc. (And I’m on vacation right now.)

– James

For the erroneous GCC behavior, see <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87150> "move ctor wrongly chosen in return stmt (derived vs. base)".

So, Clang does have a warning for deprecated copy, except that it’s buried in -Wdeprecated without its own flag. There’s a few other differences too. GCC will warn once per call to the deprecated function while Clang will warn once per deprecated function. GCC will not consider functions that are “= default” or “= delete” while Clang will. That means for “struct S { ~S() = default; };” using S’s implicit copy constructor will cause Clang to warn while GCC does not. GCC uses “user-provided” while Clang says “user-declared”

$ cat test.cc

struct S {
~S();
};

S s = s;

$ gcc test.cc -Wdeprecated-copy

test.cc:5:7: warning: implicitly-declared ‘constexpr S::S(const S&)’ is deprecated [-Wdeprecated-copy]
5 | S s = s;

^
test.cc:2:3: note: because ‘S’ has user-provided ‘S::~S()’
2 | ~S();
^

$ clang test.cc -Wdeprecated

test:2:3: warning: definition of implicit copy constructor for ‘S’ is deprecated because it has a user-declared destructor [-Wdeprecated]
~S();

^

test:5:7: note: in implicit copy constructor for ‘S’ first required here

S s = s;

^

This is a patch to give Clang a -Wdeprecated-copy, which also ignores explicitly defaulted/deleted functions like GCC.
https://reviews.llvm.org/D52860

https://reviews.llvm.org/D52863 - LLVM cleanup
https://reviews.llvm.org/D52864 - Clang cleanup

These are the needed cleanup for the new -Wdeprecated-copy warning. LatticeCell seems to be the only interesting case, with logic in the copy assignment operator not being used in the copy constructor. The rest are pretty straight forward.