[review] Add static_asserts for the sizes of space-critical structs

[review] Add static_asserts for the sizes of space-critical structs

[Christian Biesinger (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/306
......................................................................

Add static_asserts for the sizes of space-critical structs

Specifically the three structs mentioned in symtab.h:
- general_symbol_info
- symbol
- partial_symbol

This ensures that those structs won't accidentally get bigger.

gdb/ChangeLog:

2019-10-25  Christian Biesinger  <cbiesinger@google.com>

	* psympriv.h: Add static_asserts for sizeof (general_symbol_info)
	and sizeof (symbol).
	* symtab.h: Add a static_assert for sizeof (partial_symbol).

Change-Id: Idd68320aa3e79ee7cc749019724636a58ce4b9c6
---
M gdb/psympriv.h
M gdb/symtab.h
2 files changed, 11 insertions(+), 0 deletions(-)



diff --git a/gdb/psympriv.h b/gdb/psympriv.h
index 3e89742..b32311c 100644
--- a/gdb/psympriv.h
+++ b/gdb/psympriv.h
@@ -81,6 +81,10 @@
   ENUM_BITFIELD(address_class) aclass : SYMBOL_ACLASS_BITS;
 };
 
+gdb_static_assert ((sizeof (void *) == 8 && sizeof (partial_symbol) == 40)
+		   || (sizeof (void *) == 4 &&
+		       sizeof (partial_symbol) == 24));
+
 /* A convenience enum to give names to some constants used when
    searching psymtabs.  This is internal to psymtab and should not be
    used elsewhere.  */
diff --git a/gdb/symtab.h b/gdb/symtab.h
index dc65409..e02e49b 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -445,6 +445,10 @@
   short section;
 };
 
+gdb_static_assert ((sizeof (void *) == 8 && sizeof (general_symbol_info) == 32)
+		   || (sizeof (void *) == 4 &&
+		       sizeof (general_symbol_info) == 20));
+
 extern void symbol_set_demangled_name (struct general_symbol_info *,
 				       const char *,
                                        struct obstack *);
@@ -1182,6 +1186,9 @@
   struct symbol *hash_next;
 };
 
+gdb_static_assert ((sizeof (void *) == 8 && sizeof (symbol) == 72) ||
+		   (sizeof (void *) == 4 && sizeof (symbol) == 40));
+
 /* Several lookup functions return both a symbol and the block in which the
    symbol is found.  This structure is used in these cases.  */
 

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Idd68320aa3e79ee7cc749019724636a58ce4b9c6
Gerrit-Change-Number: 306
Gerrit-PatchSet: 1
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-MessageType: newchange

[review v2] Add static_asserts for the sizes of space-critical structs

[Tom Tromey (Code Review)]

Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/306
......................................................................


Patch Set 2: Code-Review+1

(1 comment)

I think these spots should maybe come with a short comment explaining
that the assert is just to make sure you don't change the size by
accident.  That way people changing the size on purpose will know
they can just update the assert.  WDYT?

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/306/2/gdb/symtab.h 
File gdb/symtab.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/306/2/gdb/symtab.h@1189 
PS2, Line 1189: 
1180 |   /* FIXME drow/2003-02-21: For the LOC_BLOCK case, it might be better
1181 |      to add a magic symbol to the block containing this information,
1182 |      or to have a generic debug info annotation slot for symbols.  */
1183 | 
1184 |   void *aux_value;
1185 | 
1186 |   struct symbol *hash_next;
1187 | };
1188 | 
1189 | gdb_static_assert ((sizeof (void *) == 8 && sizeof (symbol) == 72) ||

|| at the start of the next line.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Idd68320aa3e79ee7cc749019724636a58ce4b9c6
Gerrit-Change-Number: 306
Gerrit-PatchSet: 2
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 29 Oct 2019 19:22:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[review v3] Add static_asserts for the sizes of space-critical structs

[Christian Biesinger (Code Review)]

Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/306
......................................................................


Uploaded patch set 3: Patch Set 2 was rebased.

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/306/2/gdb/symtab.h 
File gdb/symtab.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/306/2/gdb/symtab.h@1189 
PS2, Line 1189: 
1184 |   void *aux_value;
1185 | 
1186 |   struct symbol *hash_next;
1187 | };
1188 | 
1189 > gdb_static_assert ((sizeof (void *) == 8 && sizeof (symbol) == 72) ||
1190 | 		   (sizeof (void *) == 4 && sizeof (symbol) == 40));
1191 | 
1192 | /* Several lookup functions return both a symbol and the block in which the
1193 |    symbol is found.  This structure is used in these cases.  */
1194 | 

> || at the start of the next line.

Done



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Idd68320aa3e79ee7cc749019724636a58ce4b9c6
Gerrit-Change-Number: 306
Gerrit-PatchSet: 3
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Mon, 04 Nov 2019 17:43:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: comment

[review v4] Add static_asserts for the sizes of space-critical structs

[Christian Biesinger (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/306
......................................................................

Add static_asserts for the sizes of space-critical structs

Specifically the three structs mentioned in symtab.h:
- general_symbol_info
- symbol
- partial_symbol

This ensures that those structs won't accidentally get bigger.

gdb/ChangeLog:

2019-10-25  Christian Biesinger  <cbiesinger@google.com>

	* psympriv.h: Add static_asserts for sizeof (general_symbol_info)
	and sizeof (symbol).
	* symtab.h: Add a static_assert for sizeof (partial_symbol).

Change-Id: Idd68320aa3e79ee7cc749019724636a58ce4b9c6
---
M gdb/psympriv.h
M gdb/symtab.h
2 files changed, 19 insertions(+), 0 deletions(-)



diff --git a/gdb/psympriv.h b/gdb/psympriv.h
index 19d692b..c81261a 100644
--- a/gdb/psympriv.h
+++ b/gdb/psympriv.h
@@ -82,6 +82,12 @@
   ENUM_BITFIELD(address_class) aclass : SYMBOL_ACLASS_BITS;
 };
 
+/* This struct is size-critical (see comment at the to of symtab.h), so this
+   assert makes sure the size doesn't change accidentally.  Be careful when
+   purposely increasing the size.  */
+gdb_static_assert ((sizeof (void *) == 8 && sizeof (partial_symbol) == 40)
+		   || (sizeof (void *) == 4 && sizeof (partial_symbol) == 24));
+
 /* A convenience enum to give names to some constants used when
    searching psymtabs.  This is internal to psymtab and should not be
    used elsewhere.  */
diff --git a/gdb/symtab.h b/gdb/symtab.h
index eac44ae..83b75d1 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -446,6 +446,13 @@
   short section;
 };
 
+/* This struct is size-critical (see comment at the top), so this assert
+   makes sure the size doesn't change accidentally.  Be careful when
+   purposely increasing the size.  */
+gdb_static_assert ((sizeof (void *) == 8 && sizeof (general_symbol_info) == 32)
+		   || (sizeof (void *) == 4 &&
+		       sizeof (general_symbol_info) == 20));
+
 extern void symbol_set_demangled_name (struct general_symbol_info *,
 				       const char *,
                                        struct obstack *);
@@ -1187,6 +1194,12 @@
   struct symbol *hash_next;
 };
 
+/* This struct is size-critical (see comment at the top), so this assert
+   makes sure the size doesn't change accidentally.  Be careful when
+   purposely increasing the size.  */
+gdb_static_assert ((sizeof (void *) == 8 && sizeof (symbol) == 72)
+		   || (sizeof (void *) == 4 && sizeof (symbol) == 40));
+
 /* Several lookup functions return both a symbol and the block in which the
    symbol is found.  This structure is used in these cases.  */
 

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Idd68320aa3e79ee7cc749019724636a58ce4b9c6
Gerrit-Change-Number: 306
Gerrit-PatchSet: 4
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset

[review v4] Add static_asserts for the sizes of space-critical structs

[Tom Tromey (Code Review)]

Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/306
......................................................................


Patch Set 4: Code-Review+2

Thank you.  This looks good to me.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Idd68320aa3e79ee7cc749019724636a58ce4b9c6
Gerrit-Change-Number: 306
Gerrit-PatchSet: 4
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Mon, 04 Nov 2019 17:56:34 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[pushed] Add static_asserts for the sizes of space-critical structs

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/306
......................................................................

Add static_asserts for the sizes of space-critical structs

Specifically the three structs mentioned in symtab.h:
- general_symbol_info
- symbol
- partial_symbol

This ensures that those structs won't accidentally get bigger.

gdb/ChangeLog:

2019-11-04  Christian Biesinger  <cbiesinger@google.com>

	* psympriv.h: Add static_asserts for sizeof (general_symbol_info)
	and sizeof (symbol).
	* symtab.h: Add a static_assert for sizeof (partial_symbol).

Change-Id: Idd68320aa3e79ee7cc749019724636a58ce4b9c6
---
M gdb/ChangeLog
M gdb/psympriv.h
M gdb/symtab.h
3 files changed, 25 insertions(+), 0 deletions(-)


diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7c5efce..41daef6 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-11-04  Christian Biesinger  <cbiesinger@google.com>
+
+	* psympriv.h: Add static_asserts for sizeof (general_symbol_info)
+	and sizeof (symbol).
+	* symtab.h: Add a static_assert for sizeof (partial_symbol).
+
 2019-11-04  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
 
 	* NEWS (Changes since GDB 8.3): Document Solaris 10 removal.
diff --git a/gdb/psympriv.h b/gdb/psympriv.h
index 19d692b..c81261a 100644
--- a/gdb/psympriv.h
+++ b/gdb/psympriv.h
@@ -82,6 +82,12 @@
   ENUM_BITFIELD(address_class) aclass : SYMBOL_ACLASS_BITS;
 };
 
+/* This struct is size-critical (see comment at the to of symtab.h), so this
+   assert makes sure the size doesn't change accidentally.  Be careful when
+   purposely increasing the size.  */
+gdb_static_assert ((sizeof (void *) == 8 && sizeof (partial_symbol) == 40)
+		   || (sizeof (void *) == 4 && sizeof (partial_symbol) == 24));
+
 /* A convenience enum to give names to some constants used when
    searching psymtabs.  This is internal to psymtab and should not be
    used elsewhere.  */
diff --git a/gdb/symtab.h b/gdb/symtab.h
index eac44ae..83b75d1 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -446,6 +446,13 @@
   short section;
 };
 
+/* This struct is size-critical (see comment at the top), so this assert
+   makes sure the size doesn't change accidentally.  Be careful when
+   purposely increasing the size.  */
+gdb_static_assert ((sizeof (void *) == 8 && sizeof (general_symbol_info) == 32)
+		   || (sizeof (void *) == 4 &&
+		       sizeof (general_symbol_info) == 20));
+
 extern void symbol_set_demangled_name (struct general_symbol_info *,
 				       const char *,
                                        struct obstack *);
@@ -1187,6 +1194,12 @@
   struct symbol *hash_next;
 };
 
+/* This struct is size-critical (see comment at the top), so this assert
+   makes sure the size doesn't change accidentally.  Be careful when
+   purposely increasing the size.  */
+gdb_static_assert ((sizeof (void *) == 8 && sizeof (symbol) == 72)
+		   || (sizeof (void *) == 4 && sizeof (symbol) == 40));
+
 /* Several lookup functions return both a symbol and the block in which the
    symbol is found.  This structure is used in these cases.  */
 

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Idd68320aa3e79ee7cc749019724636a58ce4b9c6
Gerrit-Change-Number: 306
Gerrit-PatchSet: 5
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: merged

Re: [pushed] Add static_asserts for the sizes of space-critical structs

[Szabolcs Nagy]

On 04/11/2019 18:14, Sourceware to Gerrit sync (Code Review) wrote:
> Sourceware to Gerrit sync has submitted this change.
> 
> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/306
> ......................................................................
> 
> Add static_asserts for the sizes of space-critical structs
> 
> Specifically the three structs mentioned in symtab.h:
> - general_symbol_info
> - symbol
> - partial_symbol
> 
> This ensures that those structs won't accidentally get bigger.
> 
> gdb/ChangeLog:
> 
> 2019-11-04  Christian Biesinger  <cbiesinger@google.com>
> 
> 	* psympriv.h: Add static_asserts for sizeof (general_symbol_info)
> 	and sizeof (symbol).
> 	* symtab.h: Add a static_assert for sizeof (partial_symbol).
> 
> Change-Id: Idd68320aa3e79ee7cc749019724636a58ce4b9c6

armhf build fails since this commit:

https://gdb-buildbot.osci.io/#/builders/29/builds/650

gdb/symtab.h:453:6: error: static assertion failed
  452 | gdb_static_assert ((sizeof (void *) == 8 && sizeof (general_symbol_info) == 32)
      |                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  453 |      || (sizeof (void *) == 4
      |      ^~~~~~~~~~~~~~~~~~~~~~~~
  454 |          && sizeof (general_symbol_info) == 20));
      |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Re: [pushed] Add static_asserts for the sizes of space-critical structs

[Christian Biesinger via gdb-patches]

On Wed, Nov 13, 2019 at 1:47 AM Szabolcs Nagy <Szabolcs.Nagy@arm.com> wrote:
>
> On 04/11/2019 18:14, Sourceware to Gerrit sync (Code Review) wrote:
> > Sourceware to Gerrit sync has submitted this change.
> >
> > Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/306
> > ......................................................................
> >
> > Add static_asserts for the sizes of space-critical structs
> >
> > Specifically the three structs mentioned in symtab.h:
> > - general_symbol_info
> > - symbol
> > - partial_symbol
> >
> > This ensures that those structs won't accidentally get bigger.
> >
> > gdb/ChangeLog:
> >
> > 2019-11-04  Christian Biesinger  <cbiesinger@google.com>
> >
> >       * psympriv.h: Add static_asserts for sizeof (general_symbol_info)
> >       and sizeof (symbol).
> >       * symtab.h: Add a static_assert for sizeof (partial_symbol).
> >
> > Change-Id: Idd68320aa3e79ee7cc749019724636a58ce4b9c6
>
> armhf build fails since this commit:
>
> https://gdb-buildbot.osci.io/#/builders/29/builds/650

Tromey's patch at
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/611 will fix
this.

Christian

>
> gdb/symtab.h:453:6: error: static assertion failed
>   452 | gdb_static_assert ((sizeof (void *) == 8 && sizeof (general_symbol_info) == 32)
>       |                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   453 |      || (sizeof (void *) == 4
>       |      ^~~~~~~~~~~~~~~~~~~~~~~~
>   454 |          && sizeof (general_symbol_info) == 20));
>       |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[pushed] Add static_asserts for the sizes of space-critical structs

[Sourceware to Gerrit sync (Code Review)]

The original change was created by Christian Biesinger.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/306
......................................................................

Add static_asserts for the sizes of space-critical structs

Specifically the three structs mentioned in symtab.h:
- general_symbol_info
- symbol
- partial_symbol

This ensures that those structs won't accidentally get bigger.

gdb/ChangeLog:

2019-11-04  Christian Biesinger  <cbiesinger@google.com>

	* psympriv.h: Add static_asserts for sizeof (general_symbol_info)
	and sizeof (symbol).
	* symtab.h: Add a static_assert for sizeof (partial_symbol).

Change-Id: Idd68320aa3e79ee7cc749019724636a58ce4b9c6
---
M gdb/ChangeLog
M gdb/psympriv.h
M gdb/symtab.h
3 files changed, 25 insertions(+), 0 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7c5efce..41daef6 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-11-04  Christian Biesinger  <cbiesinger@google.com>
+
+	* psympriv.h: Add static_asserts for sizeof (general_symbol_info)
+	and sizeof (symbol).
+	* symtab.h: Add a static_assert for sizeof (partial_symbol).
+
 2019-11-04  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
 
 	* NEWS (Changes since GDB 8.3): Document Solaris 10 removal.
diff --git a/gdb/psympriv.h b/gdb/psympriv.h
index 19d692b..c81261a 100644
--- a/gdb/psympriv.h
+++ b/gdb/psympriv.h
@@ -82,6 +82,12 @@
   ENUM_BITFIELD(address_class) aclass : SYMBOL_ACLASS_BITS;
 };
 
+/* This struct is size-critical (see comment at the to of symtab.h), so this
+   assert makes sure the size doesn't change accidentally.  Be careful when
+   purposely increasing the size.  */
+gdb_static_assert ((sizeof (void *) == 8 && sizeof (partial_symbol) == 40)
+		   || (sizeof (void *) == 4 && sizeof (partial_symbol) == 24));
+
 /* A convenience enum to give names to some constants used when
    searching psymtabs.  This is internal to psymtab and should not be
    used elsewhere.  */
diff --git a/gdb/symtab.h b/gdb/symtab.h
index eac44ae..83b75d1 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -446,6 +446,13 @@
   short section;
 };
 
+/* This struct is size-critical (see comment at the top), so this assert
+   makes sure the size doesn't change accidentally.  Be careful when
+   purposely increasing the size.  */
+gdb_static_assert ((sizeof (void *) == 8 && sizeof (general_symbol_info) == 32)
+		   || (sizeof (void *) == 4 &&
+		       sizeof (general_symbol_info) == 20));
+
 extern void symbol_set_demangled_name (struct general_symbol_info *,
 				       const char *,
                                        struct obstack *);
@@ -1187,6 +1194,12 @@
   struct symbol *hash_next;
 };
 
+/* This struct is size-critical (see comment at the top), so this assert
+   makes sure the size doesn't change accidentally.  Be careful when
+   purposely increasing the size.  */
+gdb_static_assert ((sizeof (void *) == 8 && sizeof (symbol) == 72)
+		   || (sizeof (void *) == 4 && sizeof (symbol) == 40));
+
 /* Several lookup functions return both a symbol and the block in which the
    symbol is found.  This structure is used in these cases.  */
 

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Idd68320aa3e79ee7cc749019724636a58ce4b9c6
Gerrit-Change-Number: 306
Gerrit-PatchSet: 5
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset