diff mbox

Fix PR gdb/23835: Don't redefine _FORTIFY_SOURCE if it's already defined

Message ID 20181030214236.29081-1-sergiodj@redhat.com
State New, archived
Headers

Commit Message

Sergio Durigan Junior Oct. 30, 2018, 9:42 p.m. UTC 
  Gentoo has a local GCC patch which always defines _FORTIFY_SOURCE=2.
This causes a build problem when building GDB there, because
"common/common-defs.h" also defines _FORTIFY_SOURCE=2:

    CXX    gdb.o
  In file included from ../../gdb/defs.h:28:0,
		   from ../../gdb/gdb.c:19:
  ../../gdb/common/common-defs.h:71:0: error: "_FORTIFY_SOURCE" redefined [-Werror]
   #define _FORTIFY_SOURCE 2

  <built-in>: note: this is the location of the previous definition
  cc1plus: all warnings being treated as errors
  make[2]: *** [Makefile:1619: gdb.o] Error 1

Even though it is questionable whether Gentoo's approach is the
correct one:

  https://jira.mongodb.org/browse/SERVER-29982
  https://bugs.gentoo.org/621036

it is still possible for GDB to be a bit more robust here and make
sure it just defines _FORTIFY_SOURCE if it hasn't been defined
already.  This patch does that.

Tested by rebuilding and making sure the macro was defined.

gdb/ChangeLog:
2018-10-30  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR gdb/23835
	* common/common-defs.h: Don't redefine _FORTIFY_SOURCE is it's
	already defined.
---
 gdb/ChangeLog            | 6 ++++++
 gdb/common/common-defs.h | 5 +++--
 2 files changed, 9 insertions(+), 2 deletions(-)

Comments

Simon Marchi Oct. 30, 2018, 9:53 p.m. UTC | #1 
On 2018-10-30 17:42, Sergio Durigan Junior wrote:
> Gentoo has a local GCC patch which always defines _FORTIFY_SOURCE=2.
> This causes a build problem when building GDB there, because
> "common/common-defs.h" also defines _FORTIFY_SOURCE=2:
> 
>     CXX    gdb.o
>   In file included from ../../gdb/defs.h:28:0,
> 		   from ../../gdb/gdb.c:19:
>   ../../gdb/common/common-defs.h:71:0: error: "_FORTIFY_SOURCE"
> redefined [-Werror]
>    #define _FORTIFY_SOURCE 2
> 
>   <built-in>: note: this is the location of the previous definition
>   cc1plus: all warnings being treated as errors
>   make[2]: *** [Makefile:1619: gdb.o] Error 1
> 
> Even though it is questionable whether Gentoo's approach is the
> correct one:
> 
>   https://jira.mongodb.org/browse/SERVER-29982
>   https://bugs.gentoo.org/621036
> 
> it is still possible for GDB to be a bit more robust here and make
> sure it just defines _FORTIFY_SOURCE if it hasn't been defined
> already.  This patch does that.
> 
> Tested by rebuilding and making sure the macro was defined.

I think it makes sense, it also gives the user the possibility to 
override it, if they don't like our value.  Give it a few days to give 
others a change to respond.  If you don't hear anything in ~1 week, 
please go ahead and push.

> gdb/ChangeLog:
> 2018-10-30  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	PR gdb/23835
> 	* common/common-defs.h: Don't redefine _FORTIFY_SOURCE is it's
> 	already defined.
> ---
>  gdb/ChangeLog            | 6 ++++++
>  gdb/common/common-defs.h | 5 +++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 940300f95a..239d7e16c2 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2018-10-30  Sergio Durigan Junior  <sergiodj@redhat.com>
> +
> +	PR gdb/23835
> +	* common/common-defs.h: Don't redefine _FORTIFY_SOURCE is it's

is -> if.

Simon
Joel Brobecker Oct. 30, 2018, 10:47 p.m. UTC | #2 
> > Gentoo has a local GCC patch which always defines _FORTIFY_SOURCE=2.
> > This causes a build problem when building GDB there, because
> > "common/common-defs.h" also defines _FORTIFY_SOURCE=2:
> > 
> >     CXX    gdb.o
> >   In file included from ../../gdb/defs.h:28:0,
> > 		   from ../../gdb/gdb.c:19:
> >   ../../gdb/common/common-defs.h:71:0: error: "_FORTIFY_SOURCE"
> > redefined [-Werror]
> >    #define _FORTIFY_SOURCE 2
> > 
> >   <built-in>: note: this is the location of the previous definition
> >   cc1plus: all warnings being treated as errors
> >   make[2]: *** [Makefile:1619: gdb.o] Error 1
> > 
> > Even though it is questionable whether Gentoo's approach is the
> > correct one:
> > 
> >   https://jira.mongodb.org/browse/SERVER-29982
> >   https://bugs.gentoo.org/621036
> > 
> > it is still possible for GDB to be a bit more robust here and make
> > sure it just defines _FORTIFY_SOURCE if it hasn't been defined
> > already.  This patch does that.
> > 
> > Tested by rebuilding and making sure the macro was defined.
> 
> I think it makes sense, it also gives the user the possibility to override
> it, if they don't like our value.  Give it a few days to give others a
> change to respond.  If you don't hear anything in ~1 week, please go ahead
> and push.

Good point about allowing the user to override it.

The patch looks OK to me too.
Sergio Durigan Junior Oct. 31, 2018, 8:04 p.m. UTC | #3 
On Tuesday, October 30 2018, Simon Marchi wrote:

> On 2018-10-30 17:42, Sergio Durigan Junior wrote:
>> Gentoo has a local GCC patch which always defines _FORTIFY_SOURCE=2.
>> This causes a build problem when building GDB there, because
>> "common/common-defs.h" also defines _FORTIFY_SOURCE=2:
>>
>>     CXX    gdb.o
>>   In file included from ../../gdb/defs.h:28:0,
>> 		   from ../../gdb/gdb.c:19:
>>   ../../gdb/common/common-defs.h:71:0: error: "_FORTIFY_SOURCE"
>> redefined [-Werror]
>>    #define _FORTIFY_SOURCE 2
>>
>>   <built-in>: note: this is the location of the previous definition
>>   cc1plus: all warnings being treated as errors
>>   make[2]: *** [Makefile:1619: gdb.o] Error 1
>>
>> Even though it is questionable whether Gentoo's approach is the
>> correct one:
>>
>>   https://jira.mongodb.org/browse/SERVER-29982
>>   https://bugs.gentoo.org/621036
>>
>> it is still possible for GDB to be a bit more robust here and make
>> sure it just defines _FORTIFY_SOURCE if it hasn't been defined
>> already.  This patch does that.
>>
>> Tested by rebuilding and making sure the macro was defined.
>
> I think it makes sense, it also gives the user the possibility to
> override it, if they don't like our value.  Give it a few days to give
> others a change to respond.  If you don't hear anything in ~1 week,
> please go ahead and push.

Thanks for the review.  I'll wait a week.

>> gdb/ChangeLog:
>> 2018-10-30  Sergio Durigan Junior  <sergiodj@redhat.com>
>>
>> 	PR gdb/23835
>> 	* common/common-defs.h: Don't redefine _FORTIFY_SOURCE is it's
>> 	already defined.
>> ---
>>  gdb/ChangeLog            | 6 ++++++
>>  gdb/common/common-defs.h | 5 +++--
>>  2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 940300f95a..239d7e16c2 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,9 @@
>> +2018-10-30  Sergio Durigan Junior  <sergiodj@redhat.com>
>> +
>> +	PR gdb/23835
>> +	* common/common-defs.h: Don't redefine _FORTIFY_SOURCE is it's
>
> is -> if.

Fixed.
Sergio Durigan Junior Oct. 31, 2018, 8:05 p.m. UTC | #4 
On Tuesday, October 30 2018, Joel Brobecker wrote:

>> > Gentoo has a local GCC patch which always defines _FORTIFY_SOURCE=2.
>> > This causes a build problem when building GDB there, because
>> > "common/common-defs.h" also defines _FORTIFY_SOURCE=2:
>> > 
>> >     CXX    gdb.o
>> >   In file included from ../../gdb/defs.h:28:0,
>> > 		   from ../../gdb/gdb.c:19:
>> >   ../../gdb/common/common-defs.h:71:0: error: "_FORTIFY_SOURCE"
>> > redefined [-Werror]
>> >    #define _FORTIFY_SOURCE 2
>> > 
>> >   <built-in>: note: this is the location of the previous definition
>> >   cc1plus: all warnings being treated as errors
>> >   make[2]: *** [Makefile:1619: gdb.o] Error 1
>> > 
>> > Even though it is questionable whether Gentoo's approach is the
>> > correct one:
>> > 
>> >   https://jira.mongodb.org/browse/SERVER-29982
>> >   https://bugs.gentoo.org/621036
>> > 
>> > it is still possible for GDB to be a bit more robust here and make
>> > sure it just defines _FORTIFY_SOURCE if it hasn't been defined
>> > already.  This patch does that.
>> > 
>> > Tested by rebuilding and making sure the macro was defined.
>> 
>> I think it makes sense, it also gives the user the possibility to override
>> it, if they don't like our value.  Give it a few days to give others a
>> change to respond.  If you don't hear anything in ~1 week, please go ahead
>> and push.
>
> Good point about allowing the user to override it.
>
> The patch looks OK to me too.

Thanks for the review, Joel.

Given that two global maintainers replied and approved the patch, I
assume it's OK if I go ahead and push it...?

Thanks,
Joel Brobecker Oct. 31, 2018, 9:27 p.m. UTC | #5 
> Thanks for the review, Joel.
> 
> Given that two global maintainers replied and approved the patch, I
> assume it's OK if I go ahead and push it...?

Yes, sorry. That was part of the intent of my message; it's
a small patch anyway, so if we find there are problems with it,
or objections we can't resolve quickly, it's easy to revert.
Sergio Durigan Junior Oct. 31, 2018, 9:43 p.m. UTC | #6 
On Wednesday, October 31 2018, Joel Brobecker wrote:

>> Thanks for the review, Joel.
>> 
>> Given that two global maintainers replied and approved the patch, I
>> assume it's OK if I go ahead and push it...?
>
> Yes, sorry. That was part of the intent of my message; it's
> a small patch anyway, so if we find there are problems with it,
> or objections we can't resolve quickly, it's easy to revert.

Thanks, Joel.  I pushed the patch now.

656efb5e2691b2bd29573d9985d20206c47b6927
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 940300f95a..239d7e16c2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@ 
+2018-10-30  Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	PR gdb/23835
+	* common/common-defs.h: Don't redefine _FORTIFY_SOURCE is it's
+	already defined.
+
 2018-10-30  Tom Tromey  <tom@tromey.com>
 
 	* main.c (captured_main_1): Check return value of bfd_init.
diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
index 58445b1611..86f7c1ab9a 100644
--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -65,9 +65,10 @@ 
    enable it here in order to try to catch these problems earlier;
    plus this seems like a reasonable safety measure.  The check for
    optimization is required because _FORTIFY_SOURCE only works when
-   optimization is enabled.  */
+   optimization is enabled.  If _FORTIFY_SOURCE is already defined,
+   then we don't do anything.  */
 
-#if defined __OPTIMIZE__ && __OPTIMIZE__ > 0
+#if !defined _FORTIFY_SOURCE && defined __OPTIMIZE__ && __OPTIMIZE__ > 0
 #define _FORTIFY_SOURCE 2
 #endif