[review] Do not static assert symbol size on Windows

[review] Do not static assert symbol size on Windows

[Tom Tromey (Code Review)]

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

Do not static assert symbol size on Windows

commit 3573abe1d added static asserts to ensure that symbol sizes
don't vary.  However, the Windows ABI requires a different layout than
the ABI commonly in use on Unix systems, and so these asserts fail.

This patch simply disables these asserts on Windows.

gdb/ChangeLog
2019-11-11  Tom Tromey  <tromey@adacore.com>

	* psympriv.h (partial_symbol): Don't static assert on Windows.
	* symtab.h (general_symbol_info, symbol): Don't static assert on
	Windows.

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



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index dd280ec..c95cb76 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-11-11  Tom Tromey  <tromey@adacore.com>
+
+	* psympriv.h (partial_symbol): Don't static assert on Windows.
+	* symtab.h (general_symbol_info, symbol): Don't static assert on
+	Windows.
+
 2019-11-10  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* python/py-symbol.c (gdbpy_lookup_static_symbols): New
diff --git a/gdb/psympriv.h b/gdb/psympriv.h
index c81261a..80f2a1a 100644
--- a/gdb/psympriv.h
+++ b/gdb/psympriv.h
@@ -82,11 +82,15 @@
   ENUM_BITFIELD(address_class) aclass : SYMBOL_ACLASS_BITS;
 };
 
+/* The Windows ABI requires this structure to be packed
+   differently.  */
+#ifndef _WIN32
 /* 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));
+#endif
 
 /* A convenience enum to give names to some constants used when
    searching psymtabs.  This is internal to psymtab and should not be
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 111a0f9..3aa4c45 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -446,12 +446,16 @@
   short section;
 };
 
+/* The Windows ABI requires this structure to be packed
+   differently.  */
+#ifndef _WIN32
 /* 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));
+#endif
 
 extern void symbol_set_demangled_name (struct general_symbol_info *,
 				       const char *,
@@ -1194,11 +1198,15 @@
   struct symbol *hash_next;
 };
 
+/* The Windows ABI requires this structure to be packed
+   differently.  */
+#ifndef _WIN32
 /* 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));
+#endif
 
 /* 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: I51940fb2240c474838b48494b5072081701789bb
Gerrit-Change-Number: 611
Gerrit-PatchSet: 1
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newchange

[review] Do not static assert symbol size on Windows

[Christian Biesinger (Code Review)]

Christian Biesinger has posted comments on this change.

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


Patch Set 1: Code-Review+1

Hmm, this change seems fine but I am curious about your build setup that makes this necessary? These asserts do not fail for me on Mingw 64bit with g++ 9.2


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I51940fb2240c474838b48494b5072081701789bb
Gerrit-Change-Number: 611
Gerrit-PatchSet: 1
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Comment-Date: Mon, 11 Nov 2019 16:27:24 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[review] Do not static assert symbol size on Windows

[Tom Tromey (Code Review)]

Tom Tromey has posted comments on this change.

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


Patch Set 1:

> Patch Set 1: Code-Review+1
> 
> Hmm, this change seems fine but I am curious about your build setup that makes this necessary? These asserts do not fail for me on Mingw 64bit with g++ 9.2

I built using `--host=i686-w64-mingw32` on x86-64 Fedora 29.  Otherwise, I didn't
do anything special.  So, I don't know why there is a difference here.

I think, though, that we should probably just remove these asserts.  There was
another build failure reported to bugzilla, and then we had an internal build
failure on PPC here at AdaCore.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I51940fb2240c474838b48494b5072081701789bb
Gerrit-Change-Number: 611
Gerrit-PatchSet: 1
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 12 Nov 2019 19:38:09 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review] Do not static assert symbol size on Windows

[Christian Biesinger (Code Review)]

Christian Biesinger has posted comments on this change.

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


Patch Set 1:

> Patch Set 1:
> 
> > Patch Set 1: Code-Review+1
> > 
> > Hmm, this change seems fine but I am curious about your build setup that makes this necessary? These asserts do not fail for me on Mingw 64bit with g++ 9.2
> 
> I built using `--host=i686-w64-mingw32` on x86-64 Fedora 29.  Otherwise, I didn't
> do anything special.  So, I don't know why there is a difference here.
> 
> I think, though, that we should probably just remove these asserts.  There was
> another build failure reported to bugzilla, and then we had an internal build
> failure on PPC here at AdaCore.

Yeah, sounds like that may be the best option. (ah, you built for 32 bit, that's probably the difference)


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I51940fb2240c474838b48494b5072081701789bb
Gerrit-Change-Number: 611
Gerrit-PatchSet: 1
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 12 Nov 2019 19:39:11 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] Remove symbol-related static asserts

[Tom Tromey (Code Review)]

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

Remove symbol-related static asserts

commit 3573abe1d added static asserts to ensure that symbol sizes
don't vary.  However, this failed to build on Windows, on at least one
ARM platform (see PR build/25182) and internally at AdaCore for PPC.

So, I think it is probably best to just remove these assertions,
effectively reverting 3573abe1d.

gdb/ChangeLog
2019-11-12  Tom Tromey  <tromey@adacore.com>

	PR build/25182:
	* psympriv.h (partial_symbol): Remove static assert.
	* symtab.h (general_symbol_info, symbol): Remove static assert.

Change-Id: I51940fb2240c474838b48494b5072081701789bb
---
M gdb/ChangeLog
M gdb/psympriv.h
M gdb/symtab.h
3 files changed, 6 insertions(+), 19 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 23095e0..21ab575 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-11-12  Tom Tromey  <tromey@adacore.com>
+
+	PR build/25182:
+	* psympriv.h (partial_symbol): Remove static assert.
+	* symtab.h (general_symbol_info, symbol): Remove static assert.
+
 2019-11-12  Tom Tromey  <tom@tromey.com>
 
 	* tui/tui-layout.c (show_layout): Set current_layout.
diff --git a/gdb/psympriv.h b/gdb/psympriv.h
index c81261a..19d692b 100644
--- a/gdb/psympriv.h
+++ b/gdb/psympriv.h
@@ -82,12 +82,6 @@
   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 111a0f9..eac44ae 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -446,13 +446,6 @@
   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 *);
@@ -1194,12 +1187,6 @@
   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: I51940fb2240c474838b48494b5072081701789bb
Gerrit-Change-Number: 611
Gerrit-PatchSet: 2
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset

[review v2] Remove symbol-related static asserts

[Christian Biesinger (Code Review)]

Christian Biesinger has posted comments on this change.

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


Patch Set 2: Code-Review+1


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I51940fb2240c474838b48494b5072081701789bb
Gerrit-Change-Number: 611
Gerrit-PatchSet: 2
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 12 Nov 2019 20:01:56 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[review v2] Remove symbol-related static asserts

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

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


Patch Set 2: Code-Review+2


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I51940fb2240c474838b48494b5072081701789bb
Gerrit-Change-Number: 611
Gerrit-PatchSet: 2
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Wed, 13 Nov 2019 19:33:14 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[review v2] Remove symbol-related static asserts

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

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


Patch Set 2:

(1 comment)

I agree that the asserts are best removed.

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +1,10 @@ 
| +Parent:     ed2c82c3 (Consolidate setting of current_layout)

PS2, Line 1:

I agree.

| +Author:     Tom Tromey <tromey@adacore.com>
| +AuthorDate: 2019-11-11 07:43:13 -0700
| +Commit:     Tom Tromey <tromey@adacore.com>
| +CommitDate: 2019-11-12 12:52:05 -0700
| +
| +Remove symbol-related static asserts
| +
| +commit 3573abe1d added static asserts to ensure that symbol sizes
| +don't vary.  However, this failed to build on Windows, on at least one

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I51940fb2240c474838b48494b5072081701789bb
Gerrit-Change-Number: 611
Gerrit-PatchSet: 2
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-Comment-Date: Wed, 13 Nov 2019 19:33:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[pushed] Remove symbol-related static asserts

[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/+/611
......................................................................

Remove symbol-related static asserts

commit 3573abe1d added static asserts to ensure that symbol sizes
don't vary.  However, this failed to build on Windows, on at least one
ARM platform (see PR build/25182) and internally at AdaCore for PPC.

So, I think it is probably best to just remove these assertions,
effectively reverting 3573abe1d.

gdb/ChangeLog
2019-11-13  Tom Tromey  <tromey@adacore.com>

	PR build/25182:
	* psympriv.h (partial_symbol): Remove static assert.
	* symtab.h (general_symbol_info, symbol): Remove static assert.

Change-Id: I51940fb2240c474838b48494b5072081701789bb
---
M gdb/ChangeLog
M gdb/psympriv.h
M gdb/symtab.h
3 files changed, 6 insertions(+), 19 deletions(-)


diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4f40ea8..be4c5a5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-11-13  Tom Tromey  <tromey@adacore.com>
+
+	PR build/25182:
+	* psympriv.h (partial_symbol): Remove static assert.
+	* symtab.h (general_symbol_info, symbol): Remove static assert.
+
 2019-11-12  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* gdbsupport/format.c (format_pieces::format_pieces): Support
diff --git a/gdb/psympriv.h b/gdb/psympriv.h
index c81261a..19d692b 100644
--- a/gdb/psympriv.h
+++ b/gdb/psympriv.h
@@ -82,12 +82,6 @@
   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 390aee4..a6bd3c4 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -447,13 +447,6 @@
   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 *);
@@ -1204,12 +1197,6 @@
   struct symbol *hash_next = nullptr;
 };
 
-/* 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: I51940fb2240c474838b48494b5072081701789bb
Gerrit-Change-Number: 611
Gerrit-PatchSet: 3
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: merged

[pushed] Remove symbol-related static asserts

[Sourceware to Gerrit sync (Code Review)]

The original change was created by Tom Tromey.

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

Remove symbol-related static asserts

commit 3573abe1d added static asserts to ensure that symbol sizes
don't vary.  However, this failed to build on Windows, on at least one
ARM platform (see PR build/25182) and internally at AdaCore for PPC.

So, I think it is probably best to just remove these assertions,
effectively reverting 3573abe1d.

gdb/ChangeLog
2019-11-13  Tom Tromey  <tromey@adacore.com>

	PR build/25182:
	* psympriv.h (partial_symbol): Remove static assert.
	* symtab.h (general_symbol_info, symbol): Remove static assert.

Change-Id: I51940fb2240c474838b48494b5072081701789bb
---
M gdb/ChangeLog
M gdb/psympriv.h
M gdb/symtab.h
3 files changed, 6 insertions(+), 19 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4f40ea8..be4c5a5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-11-13  Tom Tromey  <tromey@adacore.com>
+
+	PR build/25182:
+	* psympriv.h (partial_symbol): Remove static assert.
+	* symtab.h (general_symbol_info, symbol): Remove static assert.
+
 2019-11-12  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* gdbsupport/format.c (format_pieces::format_pieces): Support
diff --git a/gdb/psympriv.h b/gdb/psympriv.h
index c81261a..19d692b 100644
--- a/gdb/psympriv.h
+++ b/gdb/psympriv.h
@@ -82,12 +82,6 @@
   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 390aee4..a6bd3c4 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -447,13 +447,6 @@
   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 *);
@@ -1204,12 +1197,6 @@
   struct symbol *hash_next = nullptr;
 };
 
-/* 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: I51940fb2240c474838b48494b5072081701789bb
Gerrit-Change-Number: 611
Gerrit-PatchSet: 3
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset