[review] Do not static assert symbol size on Windows

Message ID gerrit.1573484025000.I51940fb2240c474838b48494b5072081701789bb@gnutoolchain-gerrit.osci.io
State New
Headers show
Series
  • [review] Do not static assert symbol size on Windows
Related show

Commit Message

Pedro Alves (Code Review) Nov. 11, 2019, 2:53 p.m.
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(-)




-- 
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

Comments

Pedro Alves (Code Review) Nov. 11, 2019, 4:27 p.m. | #1
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
Pedro Alves (Code Review) Nov. 12, 2019, 7:38 p.m. | #2
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
Pedro Alves (Code Review) Nov. 12, 2019, 7:39 p.m. | #3
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
Pedro Alves (Code Review) Nov. 12, 2019, 8:01 p.m. | #4
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
Pedro Alves (Code Review) Nov. 13, 2019, 7:33 p.m. | #5
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
Pedro Alves (Code Review) Nov. 13, 2019, 7:33 p.m. | #6
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

Patch

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.  */