[llvm-dev] New warnings when building trunk with GCC 9

[Dávid Bolvanský via llvm-dev]

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<T>::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::ArrayRef<llvm::LazyCallGraph::Node*>::Data’ from ‘std::initializer_list<llvm::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 ‘<anonymous>’

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<M>::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<M> 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(); }

[David Blaikie via llvm-dev]

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@)?

_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

[Stephan Bergmann via llvm-dev]

On 13/09/2018 18:22, David Blaikie via llvm-dev wrote:
> On Thu, Sep 13, 2018 at 12:13 AM Dávid Bolvanský via llvm-dev
> <llvm...@lists.llvm.org <mailto:llvm...@lists.llvm.org>> wrote:
> /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. 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".)

[David Blaikie via llvm-dev]

On Fri, Sep 14, 2018 at 12:48 AM Stephan Bergmann <sber...@redhat.com> wrote:

On 13/09/2018 18:22, David Blaikie via llvm-dev wrote:
> On Thu, Sep 13, 2018 at 12:13 AM Dávid Bolvanský via llvm-dev
> <llvm...@lists.llvm.org <mailto:llvm...@lists.llvm.org>> wrote:
>     /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.
 

[Dávid Bolvanský via llvm-dev]

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 <dbla...@gmail.com> napísal(a):

[David Blaikie via llvm-dev]

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?

[Richard Smith via llvm-dev]

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.)

_______________________________________________
cfe-dev mailing list
cfe...@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

[Richard Trieu via llvm-dev]

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

[Richard Trieu via llvm-dev]

+ 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 <iostream>

#include <memory>

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();

}

[Richard Smith via llvm-dev]

On Mon, 24 Sep 2018, 20:40 Richard Trieu via cfe-dev, <cfe...@lists.llvm.org> wrote:

+ 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.

[Stephan Bergmann via llvm-dev]

On 25/09/2018 05:40, Richard Trieu via cfe-dev wrote:
> 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.

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)".

[James Dennett via llvm-dev]

On Tue, Sep 25, 2018 at 5:08 AM Richard Smith via cfe-dev <cfe...@lists.llvm.org> wrote:

On Mon, 24 Sep 2018, 20:40 Richard Trieu via cfe-dev, <cfe...@lists.llvm.org> wrote:

+ 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 

[Richard Trieu via llvm-dev]

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

[Richard Trieu via llvm-dev]

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.