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

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

[Simon Marchi]

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

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

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 3403 bytes --]

On 2/18/20 9:27 PM, Simon Marchi wrote:
> 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.
> 

Okay, works for me, so then I added just one short comment at each site.

Is it OK for trunk?


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-build-with-gcc-4.8.x.patch --]
[-- Type: text/x-patch; name="0001-Fix-build-with-gcc-4.8.x.patch", Size: 3980 bytes --]

From 43e6c14091dee9c32fe1dee62466450e57f6c13a Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Sun, 16 Feb 2020 21:43:33 +0100
Subject: [PATCH] Fix build with gcc-4.8.x

Use an explicit conversion from unique_ptr<T> to
displaced_step_closure_up to avoid a compiler bug
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>

	* aarch64-tdep.c (aarch64_displaced_step_copy_insn): Use an explicit
	conversion.
	* amd64-tdep.c (amd64_displaced_step_copy_insn): Likewise.
	* arm-linux-tdep.c (arm_linux_displaced_step_copy_insn): Likewise.
	* i386-tdep.c (i386_displaced_step_copy_insn): Likewise.
	* rs6000-tdep.c (ppc_displaced_step_copy_insn): Likewise.
	* s390-tdep.c (s390_displaced_step_copy_insn): Likewise.
---
 gdb/aarch64-tdep.c   | 3 ++-
 gdb/amd64-tdep.c     | 3 ++-
 gdb/arm-linux-tdep.c | 3 ++-
 gdb/i386-tdep.c      | 3 ++-
 gdb/rs6000-tdep.c    | 3 ++-
 gdb/s390-tdep.c      | 3 ++-
 6 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index bfacfb0..31b90c8 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -3053,7 +3053,8 @@ aarch64_displaced_step_copy_insn (struct gdbarch *gdbarch,
       dsc = NULL;
     }
 
-  return dsc;
+  /* This is a work around for a problem with g++ 4.8.  */
+  return displaced_step_closure_up (dsc.release ());
 }
 
 /* Implement the "displaced_step_fixup" gdbarch method.  */
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 9264fe4..5c56a97 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -1511,7 +1511,8 @@ amd64_displaced_step_copy_insn (struct gdbarch *gdbarch,
       displaced_step_dump_bytes (gdb_stdlog, buf, len);
     }
 
-  return dsc;
+  /* This is a work around for a problem with g++ 4.8.  */
+  return displaced_step_closure_up (dsc.release ());
 }
 
 static int
diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index ccb556b..f60cb51 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -1131,7 +1131,8 @@ arm_linux_displaced_step_copy_insn (struct gdbarch *gdbarch,
 
   arm_displaced_init_closure (gdbarch, from, to, dsc.get ());
 
-  return dsc;
+  /* This is a work around for a problem with g++ 4.8.  */
+  return displaced_step_closure_up (dsc.release ());
 }
 
 /* Implementation of `gdbarch_stap_is_single_operand', as defined in
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 9771421..19876c3 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -831,7 +831,8 @@ i386_displaced_step_copy_insn (struct gdbarch *gdbarch,
       displaced_step_dump_bytes (gdb_stdlog, buf, len);
     }
 
-  return closure;
+  /* This is a work around for a problem with g++ 4.8.  */
+  return displaced_step_closure_up (closure.release ());
 }
 
 /* Fix up the state of registers and memory after having single-stepped
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 513ce6a..2c41e1c 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -894,7 +894,8 @@ ppc_displaced_step_copy_insn (struct gdbarch *gdbarch,
       displaced_step_dump_bytes (gdb_stdlog, buf, len);
     }
 
-  return closure;
+  /* This is a work around for a problem with g++ 4.8.  */
+  return displaced_step_closure_up (closure.release ());
 }
 
 /* Fix up the state of registers and memory after having single-stepped
diff --git a/gdb/s390-tdep.c b/gdb/s390-tdep.c
index 51d0203..d8c28c7 100644
--- a/gdb/s390-tdep.c
+++ b/gdb/s390-tdep.c
@@ -477,7 +477,8 @@ s390_displaced_step_copy_insn (struct gdbarch *gdbarch,
       displaced_step_dump_bytes (gdb_stdlog, buf, len);
     }
 
-  return closure;
+  /* This is a work around for a problem with g++ 4.8.  */
+  return displaced_step_closure_up (closure.release ());
 }
 
 /* Fix up the state of registers and memory after having single-stepped
-- 
1.9.1

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

[Simon Marchi]

On 2020-02-18 4:14 p.m., Bernd Edlinger wrote:
> Okay, works for me, so then I added just one short comment at each site.
> 
> Is it OK for trunk?
> 
> 
> Thanks
> Bernd.
> 

Yes, thanks!

Simon