diff mbox

[?] Fix gdb build with gcc-4.8.x

Message ID AM6PR03MB5170CEC011682CCA1ABF44C6E4110@AM6PR03MB5170.eurprd03.prod.outlook.com
State New
Headers show

Commit Message

Bernd Edlinger Feb. 18, 2020, 7:06 p.m. UTC
Hi,

I noticed that gdb cannot be built any more with gcc-4.8.4
since Simon's patch which introduced the
std::unique_ptr<displaced_step_closure>.

The failure mode is as follows:

  CXX    amd64-tdep.o
../../binutils-gdb/gdb/amd64-tdep.c: In function ‘displaced_step_closure_up amd64_displaced_step_copy_insn(gdbarch*, CORE_ADDR, CORE_ADDR, regcache*)’:
../../binutils-gdb/gdb/amd64-tdep.c:1514:10: error: cannot bind ‘std::unique_ptr<amd64_displaced_step_closure>’ lvalue to ‘std::unique_ptr<amd64_displaced_step_closure>&&’
   return dsc;
          ^
In file included from /usr/include/c++/4.8/memory:81:0,
                 from ../../binutils-gdb/gdb/../gdbsupport/common-exceptions.h:25,
                 from ../../binutils-gdb/gdb/../gdbsupport/common-defs.h:140,
                 from ../../binutils-gdb/gdb/defs.h:28,
                 from ../../binutils-gdb/gdb/amd64-tdep.c:22:
/usr/include/c++/4.8/bits/unique_ptr.h:169:2: error:   initializing argument 1 of ‘std::unique_ptr<_Tp, _Dp>::unique_ptr(std::unique_ptr<_Up, _Ep>&&) [with _Up = amd64_displaced_step_closure; _Ep = std::default_delete<amd64_displaced_step_closure>; <template-parameter-2-3> = void; _Tp = displaced_step_closure; _Dp = std::default_delete<displaced_step_closure>]’
  unique_ptr(unique_ptr<_Up, _Ep>&& __u) noexcept
  ^
../../binutils-gdb/gdb/amd64-tdep.c:1515:1: error: control reaches end of non-void function [-Werror=return-type]
 }
 ^
cc1plus: all warnings being treated as errors


It continues to work with gcc-5.4.0, though.  I don't know what
is with gcc-4.9.x.

I have two possible workarounds for this attached to this as
variant-1 patch and variant-2 patch respectively.  I personally
would prefer variant-2 which makes displaced_step_closure_up a
wrapper class around unique_ptr<displaced_step_closure> and
avoids to trigger this compiler bug by being slightly simpler as
the original, I think the issue always starts when the argument
to displaced_step_closure_up (std::unique_ptr<T> &up) is
using double-ampersand.

So we have three possible ways to deal with this:
variant-1: simplify the code where the type cast happens,
variant-2: use a simplified wrapper clase, and
variant-3: do nothing about it, and document that gcc-5.4.0 is
or newer is required.

What do you think?


Thanks
Bernd.

Comments

Simon Marchi Feb. 18, 2020, 8:27 p.m. UTC | #1
On 2020-02-18 2:06 p.m., Bernd Edlinger wrote:
> Hi,
> 
> I noticed that gdb cannot be built any more with gcc-4.8.4
> since Simon's patch which introduced the
> std::unique_ptr<displaced_step_closure>.
> 
> The failure mode is as follows:
> 
>   CXX    amd64-tdep.o
> ../../binutils-gdb/gdb/amd64-tdep.c: In function ‘displaced_step_closure_up amd64_displaced_step_copy_insn(gdbarch*, CORE_ADDR, CORE_ADDR, regcache*)’:
> ../../binutils-gdb/gdb/amd64-tdep.c:1514:10: error: cannot bind ‘std::unique_ptr<amd64_displaced_step_closure>’ lvalue to ‘std::unique_ptr<amd64_displaced_step_closure>&&’
>    return dsc;
>           ^
> In file included from /usr/include/c++/4.8/memory:81:0,
>                  from ../../binutils-gdb/gdb/../gdbsupport/common-exceptions.h:25,
>                  from ../../binutils-gdb/gdb/../gdbsupport/common-defs.h:140,
>                  from ../../binutils-gdb/gdb/defs.h:28,
>                  from ../../binutils-gdb/gdb/amd64-tdep.c:22:
> /usr/include/c++/4.8/bits/unique_ptr.h:169:2: error:   initializing argument 1 of ‘std::unique_ptr<_Tp, _Dp>::unique_ptr(std::unique_ptr<_Up, _Ep>&&) [with _Up = amd64_displaced_step_closure; _Ep = std::default_delete<amd64_displaced_step_closure>; <template-parameter-2-3> = void; _Tp = displaced_step_closure; _Dp = std::default_delete<displaced_step_closure>]’
>   unique_ptr(unique_ptr<_Up, _Ep>&& __u) noexcept
>   ^
> ../../binutils-gdb/gdb/amd64-tdep.c:1515:1: error: control reaches end of non-void function [-Werror=return-type]
>  }
>  ^
> cc1plus: all warnings being treated as errors
> 
> 
> It continues to work with gcc-5.4.0, though.  I don't know what
> is with gcc-4.9.x.
> 
> I have two possible workarounds for this attached to this as
> variant-1 patch and variant-2 patch respectively.  I personally
> would prefer variant-2 which makes displaced_step_closure_up a
> wrapper class around unique_ptr<displaced_step_closure> and
> avoids to trigger this compiler bug by being slightly simpler as
> the original, I think the issue always starts when the argument
> to displaced_step_closure_up (std::unique_ptr<T> &up) is
> using double-ampersand.
> 
> So we have three possible ways to deal with this:
> variant-1: simplify the code where the type cast happens,
> variant-2: use a simplified wrapper clase, and
> variant-3: do nothing about it, and document that gcc-5.4.0 is
> or newer is required.
> 
> What do you think?
> 
> 
> Thanks
> Bernd.
> 

Hi Bernd,

I don't like variant 2, because it changes the API/contract of
std::unique_ptr.  It allows doing

  std::unique_ptr<amd64_displaced_step_closure> dsc;
  displaced_step_closure_up hello (dsc);

Which would not be possible if displaced_step_closure_up was
a simple typedef.  In our code base, the types that end with _up
are known to be typedefs to std::unique_ptr, and I don't think it
would be a good idea to provide such a type with a semantic that
differs from std::unique_ptr.

So I would prefer something along the lines of variant 1, but with
a small comment at each site saying that this is to work around
a problem with g++ 4.8.

Thanks,

Simon
diff mbox

Patch

From 493f67826232f0c4dab3a6bf69f851d01616e75f Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Sun, 16 Feb 2020 22:55:55 +0100
Subject: [PATCH] Fix build with gcc-4.8.x
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Make displaced_step_closure_up a wrapper class around
unique_ptr<displaced_step_closure> to avoid compiler
bugs with implicit casting from derived
unique_ptr<arch-specific data> to displaced_step_closure_up

observed with with gcc-4.8.4:

../../binutils-gdb/gdb/amd64-tdep.c:1514:10: error: cannot bind
   ‘std::unique_ptr<amd64_displaced_step_closure>’ lvalue to
   ‘std::unique_ptr<amd64_displaced_step_closure>&&’

gdb:
2020-02-16  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* infrun (displaced_step_closure_up): Convert to a wrapper class.
---
 gdb/infrun.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/gdb/infrun.h b/gdb/infrun.h
index 625c53a..49566ed 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -265,7 +265,23 @@  struct displaced_step_closure
   virtual ~displaced_step_closure () = 0;
 };
 
-using displaced_step_closure_up = std::unique_ptr<displaced_step_closure>;
+/* A wrapper class around std::unique_ptr<displaced_step_closure>.
+   Technically unnecessary, but helps avoiding a compiler bug.  */
+
+struct displaced_step_closure_up : std::unique_ptr<displaced_step_closure>
+{
+  displaced_step_closure_up ()
+  {}
+
+  template <typename T>
+  displaced_step_closure_up (std::unique_ptr<T> &up)
+  : std::unique_ptr<displaced_step_closure> (up.release ())
+  {}
+
+  displaced_step_closure_up (displaced_step_closure *up)
+  : std::unique_ptr<displaced_step_closure> (up)
+  {}
+};
 
 /* A simple displaced step closure that contains only a byte buffer.  */
 
-- 
1.9.1