Fix build failure on macOS

Message ID 20191218150518.8540-1-tromey@adacore.com
State New
Headers show
Series
  • Fix build failure on macOS
Related show

Commit Message

Tom Tromey Dec. 18, 2019, 3:05 p.m.
PR build/25250 notes that the gdb 9 pre-release fails to build on
macOS, due to a name clash between field_kind::STRING and the STRING
token in ada-exp.y.  I am not sure (I couldn't reproduce this myself),
but presumably this is due to differences caused by the version of
bison in use there.

This patch works around the problem by renaming the field_kind
enumerator.  I chose to rename this one because it is used in
relatively few places -- it's just an implementation detail of the
style code.

Let me know what you think.  I intend to check this in on the gdb 9
branch as well.

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

	PR build/25250:
	* ui-out.c (ui_out::vmessage): Update.
	* ui-out.h (enum class) <FIELD_STRING>: Rename from STRING.
	(string_field): Update.

Change-Id: Iae9f36f1b793e22c61fee0de2ab2d508668ee7e4
---
 gdb/ChangeLog | 7 +++++++
 gdb/ui-out.c  | 2 +-
 gdb/ui-out.h  | 6 ++++--
 3 files changed, 12 insertions(+), 3 deletions(-)

-- 
2.21.0

Comments

Pedro Alves Dec. 18, 2019, 4:21 p.m. | #1
On 12/18/19 3:05 PM, Tom Tromey wrote:

> Let me know what you think.  I intend to check this in on the gdb 9

> branch as well.


Whoops, should have thought of this.

> @@ -78,7 +78,9 @@ enum ui_out_type

>  enum class field_kind

>    {

>      SIGNED,

> -    STRING,

> +    /* This has a funny name to avoid clashes with tokens named

> +       "STRING".  See PR build/25250.  */

> +    FIELD_STRING,

>    };


I'd rather rename SIGNED to FIELD_SIGNED as well, for consistency.
It no longer looks funny that way.

Thanks,
Pedro Alves
Simon Marchi Dec. 18, 2019, 4:31 p.m. | #2
On 2019-12-18 10:05 a.m., Tom Tromey wrote:
> PR build/25250 notes that the gdb 9 pre-release fails to build on

> macOS, due to a name clash between field_kind::STRING and the STRING

> token in ada-exp.y.  I am not sure (I couldn't reproduce this myself),

> but presumably this is due to differences caused by the version of

> bison in use there.

> 

> This patch works around the problem by renaming the field_kind

> enumerator.  I chose to rename this one because it is used in

> relatively few places -- it's just an implementation detail of the

> style code.

> 

> Let me know what you think.  I intend to check this in on the gdb 9

> branch as well.


The difference between macOS's bison (2.3) and later version (I'm not sure at
which release the behavior changed) is that the order of the prologue (where
we #include stuff) versus the bison declarations (#define STRING xyz) changed.
Bison 2.3 puts the declarations before the prologue, which can break included
files as we see here.  Later versions put the prologue before (actually the
exact location of the code can be controlled by using %code).

I don't really mind this particular fix, as it doesn't add complexity.  But if
in the future the macOS version gives us some more major headache, I would be
comfortable just saying that we don't support it, and adding a %require statement
in the grammar file.  Recent bison versions are easily available through Homebrew
and macports.

Simon
Tom Tromey Dec. 18, 2019, 4:35 p.m. | #3
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> I'd rather rename SIGNED to FIELD_SIGNED as well, for consistency.
Pedro> It no longer looks funny that way.

Here's v2.

Tom

commit 445716fd7ce9d59ec463047fb441b63a52544581
Author: Tom Tromey <tromey@adacore.com>
Date:   Wed Dec 18 08:03:01 2019 -0700

    Fix build failure on macOS
    
    PR build/25250 notes that the gdb 9 pre-release fails to build on
    macOS, due to a name clash between field_kind::STRING and the STRING
    token in ada-exp.y.  I am not sure (I couldn't reproduce this myself),
    but presumably this is due to differences caused by the version of
    bison in use there.
    
    This patch works around the problem by renaming the field_kind
    enumerator.  I chose to rename this one because it is used in
    relatively few places -- it's just an implementation detail of the
    style code.
    
    This version also renames field_kind::SIGNED for consistency.
    
    Let me know what you think.  I intend to check this in on the gdb 9
    branch as well.
    
    gdb/ChangeLog
    2019-12-18  Tom Tromey  <tromey@adacore.com>
    
            PR build/25250:
            * ui-out.c (ui_out::vmessage): Update.
            * ui-out.h (enum class field_kind) <FIELD_STRING, FIELD_SIGNED>:
            Rename.
            (string_field): Update.
            (signed_field): Update.
    
    Change-Id: Iae9f36f1b793e22c61fee0de2ab2d508668ee7e4

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 91b81f242c0..8c74bbc1e00 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+2019-12-18  Tom Tromey  <tromey@adacore.com>
+
+	PR build/25250:
+	* ui-out.c (ui_out::vmessage): Update.
+	* ui-out.h (enum class field_kind) <FIELD_STRING, FIELD_SIGNED>:
+	Rename.
+	(string_field): Update.
+	(signed_field): Update.
+
 2019-12-17  Christian Biesinger  <cbiesinger@google.com>
 
 	* bsd-kvm.c: Include gdbsupport/pathstuff.h.
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 80845f4bcaa..2b3b7e4accb 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -730,13 +730,13 @@ ui_out::vmessage (const ui_file_style &in_style, const char *format,
 		base_field_s *bf = va_arg (args, base_field_s *);
 		switch (bf->kind)
 		  {
-		  case field_kind::SIGNED:
+		  case field_kind::FIELD_SIGNED:
 		    {
 		      auto *f = (signed_field_s *) bf;
 		      field_signed (f->name, f->val);
 		    }
 		    break;
-		  case field_kind::STRING:
+		  case field_kind::FIELD_STRING:
 		    {
 		      auto *f = (string_field_s *) bf;
 		      field_string (f->name, f->str);
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 5c96a7825be..c3ef8a57006 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -77,8 +77,11 @@ enum ui_out_type
 /* The possible kinds of fields.  */
 enum class field_kind
   {
-    SIGNED,
-    STRING,
+    /* "FIELD_STRING" needs has a funny name to avoid clashes with
+       tokens named "STRING".  See PR build/25250.  FIELD_SIGNED is
+       given a similar name for consistency.  */
+    FIELD_SIGNED,
+    FIELD_STRING,
   };
 
 /* The base type of all fields that can be emitted using %pF.  */
@@ -105,7 +108,7 @@ signed_field (const char *name, LONGEST val,
 	      signed_field_s &&tmp = {})
 {
   tmp.name = name;
-  tmp.kind = field_kind::SIGNED;
+  tmp.kind = field_kind::FIELD_SIGNED;
   tmp.val = val;
   return &tmp;
 }
@@ -126,7 +129,7 @@ string_field (const char *name, const char *str,
 	      string_field_s &&tmp = {})
 {
   tmp.name = name;
-  tmp.kind = field_kind::STRING;
+  tmp.kind = field_kind::FIELD_STRING;
   tmp.str = str;
   return &tmp;
 }
Tom Tromey Dec. 18, 2019, 4:36 p.m. | #4
Simon> I don't really mind this particular fix, as it doesn't add complexity.  But if
Simon> in the future the macOS version gives us some more major headache, I would be
Simon> comfortable just saying that we don't support it, and adding a %require statement
Simon> in the grammar file.

This sounds reasonable to me.  I imagine it's inevitable as macOS
doesn't update GNU tools any more.

thanks,
Tom
Pedro Alves Dec. 18, 2019, 4:38 p.m. | #5
On 12/18/19 4:35 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

> 

> Pedro> I'd rather rename SIGNED to FIELD_SIGNED as well, for consistency.

> Pedro> It no longer looks funny that way.

> 

> Here's v2.


OK.

Thanks,
Pedro Alves
Pedro Alves via Gdb-patches Dec. 18, 2019, 7:17 p.m. | #6
On Wed, Dec 18, 2019 at 10:35 AM Tom Tromey <tromey@adacore.com> wrote:
> --- a/gdb/ui-out.h

> +++ b/gdb/ui-out.h

> @@ -77,8 +77,11 @@ enum ui_out_type

>  /* The possible kinds of fields.  */

>  enum class field_kind

>    {

> -    SIGNED,

> -    STRING,

> +    /* "FIELD_STRING" needs has a funny name to avoid clashes with


Remove either "needs" or "has"?

It is very unfortunate that a member of an enum class will need a
prefix, since the entire point of an enum class is not to need that :(

Christian
Tom Tromey Dec. 19, 2019, 6:11 p.m. | #7
>>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:


Christian> On Wed, Dec 18, 2019 at 10:35 AM Tom Tromey <tromey@adacore.com> wrote:
>> --- a/gdb/ui-out.h

>> +++ b/gdb/ui-out.h

>> @@ -77,8 +77,11 @@ enum ui_out_type

>> /* The possible kinds of fields.  */

>> enum class field_kind

>> {

>> -    SIGNED,

>> -    STRING,

>> +    /* "FIELD_STRING" needs has a funny name to avoid clashes with


Christian> Remove either "needs" or "has"?

Ugh, sorry.  I'll fix it.

Christian> It is very unfortunate that a member of an enum class will need a
Christian> prefix, since the entire point of an enum class is not to need that :(

Yeah.  I have a dream of not using bison at all, but that's a lot of
work for a pretty small benefit in the end.

Also sometimes I wonder if we want to continue using all caps for "enum
class" constants.

Tom

Patch

diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 80845f4bcaa..6572546f26c 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -736,7 +736,7 @@  ui_out::vmessage (const ui_file_style &in_style, const char *format,
 		      field_signed (f->name, f->val);
 		    }
 		    break;
-		  case field_kind::STRING:
+		  case field_kind::FIELD_STRING:
 		    {
 		      auto *f = (string_field_s *) bf;
 		      field_string (f->name, f->str);
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 5c96a7825be..0bdb6b8b7d5 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -78,7 +78,9 @@  enum ui_out_type
 enum class field_kind
   {
     SIGNED,
-    STRING,
+    /* This has a funny name to avoid clashes with tokens named
+       "STRING".  See PR build/25250.  */
+    FIELD_STRING,
   };
 
 /* The base type of all fields that can be emitted using %pF.  */
@@ -126,7 +128,7 @@  string_field (const char *name, const char *str,
 	      string_field_s &&tmp = {})
 {
   tmp.name = name;
-  tmp.kind = field_kind::STRING;
+  tmp.kind = field_kind::FIELD_STRING;
   tmp.str = str;
   return &tmp;
 }