diff mbox

[RFA,v2,10/17] C++ify mi_parse

Message ID 77601650-1ad9-dc6e-b7a2-814a08f9348d@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves April 12, 2017, 6:37 p.m. UTC 
  On 04/12/2017 07:05 PM, Tom Tromey wrote:
> Pedro> Thanks.  Totally fine with me.  If you prefer, using in-class
> Pedro> initialization is also fine.  (We've been using it more in recent
> Pedro> patches.)
> 
> I just left it as-is, but not for any good reason.

That's totally fine with me.

FYI, I just now tried the hack below against master, and
that caught a few other cases that shouldn't have been
using memset for initialization.

struct bp_location, struct inferior, struct btrace_insn
$ make -k 2>&1 | grep "no matching.*pod_only_memset"
src/gdb/inferior.c:132:32: error: no matching function for call to ‘pod_only_memset(inferior*&, int, long unsigned int)’
src/gdb/btrace.c:1153:42: error: no matching function for call to ‘pod_only_memset(btrace_insn*, int, long unsigned int)’
src/gdb/breakpoint.c:951:53: error: no matching function for call to ‘pod_only_memset(bp_location*, int, long unsigned int)’
src/gdb/breakpoint.c:7325:32: error: no matching function for call to ‘pod_only_memset(bp_location*&, int, long unsigned int)’

I've already posted a patch to fix struct inferior:
  https://sourceware.org/ml/gdb-patches/2017-04/msg00298.html

I hadn't realized it already wasn't a POD.  Looks like that
happened because inferior has an enum_flags member, and that
one was recently made non-POD (gained a user-defined ctor to
default-zero-initialize).

I'll take a look at the others...

I wonder how bad would it be to put this hack in master.  Guess
we could always add it behind an #if 0 at least, to make it easy
to enable for quick checking?

From f8b5c40ff07891eb607dba4233419ca4e4295d22 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 12 Apr 2017 19:28:40 +0100
Subject: [PATCH] Make the compiler error out with memset on non-POD types

---
 gdb/common/common-defs.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Tom Tromey April 12, 2017, 7:25 p.m. UTC | #1 
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> FYI, I just now tried the hack below against master, and
Pedro> that caught a few other cases that shouldn't have been
Pedro> using memset for initialization.
[...]
Pedro> I wonder how bad would it be to put this hack in master.  Guess
Pedro> we could always add it behind an #if 0 at least, to make it easy
Pedro> to enable for quick checking?

I think it would be helpful if the compiler could warn about missing
member initializations in a constructor.  That would make something like
the mi_parse change more robust, which I think is the reason for the
memsets in the first place.

Tom
Pedro Alves April 13, 2017, 2:36 a.m. UTC | #2 
On 04/12/2017 08:25 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> FYI, I just now tried the hack below against master, and
> Pedro> that caught a few other cases that shouldn't have been
> Pedro> using memset for initialization.
> [...]
> Pedro> I wonder how bad would it be to put this hack in master.  Guess
> Pedro> we could always add it behind an #if 0 at least, to make it easy
> Pedro> to enable for quick checking?
> 
> I think it would be helpful if the compiler could warn about missing
> member initializations in a constructor.

Yeah, that would be nice.  In principle, -Wuninitialized would catch
those too, and be quiet if you have some field that you do want to
be left uninitialized in the ctor (e.g., some big array lazily
filled in).  (And for OOL ctors, there's -flto.)
There are unfortunately a number of bugs with it, though.
The last I ran into was:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79658

> That would make something like
> the mi_parse change more robust, which I think is the reason for the
> memsets in the first place.

Yeah.  The memsets become invalid as soon as your ctor
does something non-trivial though, we really need to zap them
sooner than later.  My current thinking is that in-class initialization
helps by making it easier to check whether all fields have default
initialization.

I've posted a series fixing the issues the hack caught, and some more,
along with a better version of the hack, here:

 https://sourceware.org/ml/gdb-patches/2017-04/msg00378.html

Thanks,
Pedro Alves
Tom Tromey April 13, 2017, 1:28 p.m. UTC | #3 
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Yeah.  The memsets become invalid as soon as your ctor
Pedro> does something non-trivial though, we really need to zap them
Pedro> sooner than later.

Yes, I agree.

Tom
diff mbox

Patch

diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
index af37111..fe94000 100644
--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -90,4 +90,20 @@ 
 /* Pull in gdb::unique_xmalloc_ptr.  */
 #include "common/gdb_unique_ptr.h"
 
+#include <type_traits>
+
+/* Redefine memset to only work with POD types.  This catches
+   initialization of structs that should have been converted to
+   ctors.  */
+template <typename T,
+	  typename = typename std::enable_if<std::is_pod<T>::value
+					     || std::is_void<T>::value>::type>
+static inline void *
+pod_only_memset (T *s, int c, size_t n)
+{
+  return memset (s, c, n);
+}
+
+#define memset pod_only_memset
+
 #endif /* COMMON_DEFS_H */