[patch] explicitly specify -std=gnu89 for gdb.cp/inline-break.exp

[patch] explicitly specify -std=gnu89 for gdb.cp/inline-break.exp

[David Blaikie]

[-- Attachment #1: Type: text/plain, Size: 290 bytes --]

This test is intending to use gnu style inline rather than the
standard c99 inline semantics. Clang defaults to c99 and the test
breaks for this (and other - there's an inlining debug info quality
bug here too - I'll file a bug and kfail the remaining failures in a
separate patch) reason.

[-- Attachment #2: inline-c89.diff --]
[-- Type: text/plain, Size: 1229 bytes --]

commit 69f01ca2de4a42f336f2656379b90d9a8b6894af
Author: David Blaikie <dblaikie@gmail.com>
Date:   Fri Apr 11 12:45:37 2014 -0700

    Compile inline test with -std=gnu89 explicitly to override Clang's default (-std=c99)
    
    gdb/testsuite/
    	* gdb.opt/inline-break.exp: Explicitly specify -std=gnu89 to
    	override Clang's default.

diff --git gdb/testsuite/ChangeLog gdb/testsuite/ChangeLog
index ed448c5..f7a4b8e 100644
--- gdb/testsuite/ChangeLog
+++ gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-04-11  David Blaikie  <dblaikie@gmail.com>
+
+	* gdb.opt/inline-break.exp: Explicitly specify -std=gnu89 to
+	override Clang's default.
+
 2014-04-11  Joel Brobecker  <brobecker@adacore.com>
 
 	Revert the following changes (regressions):
diff --git gdb/testsuite/gdb.opt/inline-break.exp gdb/testsuite/gdb.opt/inline-break.exp
index 21c958a..7eef10a 100644
--- gdb/testsuite/gdb.opt/inline-break.exp
+++ gdb/testsuite/gdb.opt/inline-break.exp
@@ -20,7 +20,7 @@
 standard_testfile
 
 if { [prepare_for_testing $testfile.exp $testfile $srcfile \
-          {debug optimize=-O2 additional_flags=-Winline}] } {
+          {debug optimize=-O2 additional_flags=-Winline additional_flags=-std=gnu89}] } {
     return -1
 }
 

Re: [patch] explicitly specify -std=gnu89 for gdb.cp/inline-break.exp

[Doug Evans]

On Fri, Apr 11, 2014 at 4:58 PM, David Blaikie <dblaikie@gmail.com> wrote:
> This test is intending to use gnu style inline rather than the
> standard c99 inline semantics. Clang defaults to c99 and the test
> breaks for this (and other - there's an inlining debug info quality
> bug here too - I'll file a bug and kfail the remaining failures in a
> separate patch) reason.

LGTM, but add a comment in the code explaining why the gnu89 is there.

Re: [patch] explicitly specify -std=gnu89 for gdb.cp/inline-break.exp

[David Blaikie]

On Fri, Apr 11, 2014 at 5:18 PM, Doug Evans <dje@google.com> wrote:
> On Fri, Apr 11, 2014 at 4:58 PM, David Blaikie <dblaikie@gmail.com> wrote:
>> This test is intending to use gnu style inline rather than the
>> standard c99 inline semantics. Clang defaults to c99 and the test
>> breaks for this (and other - there's an inlining debug info quality
>> bug here too - I'll file a bug and kfail the remaining failures in a
>> separate patch) reason.
>
> LGTM, but add a comment in the code explaining why the gnu89 is there.

Thanks. Added a comment and committed in
f180a1fb463a6a9ab4a883374120d16770486914.

Re: [patch] explicitly specify -std=gnu89 for gdb.cp/inline-break.exp

[Andrew Pinski]

On Fri, Apr 11, 2014 at 4:58 PM, David Blaikie <dblaikie@gmail.com> wrote:
> This test is intending to use gnu style inline rather than the
> standard c99 inline semantics. Clang defaults to c99 and the test
> breaks for this (and other - there's an inlining debug info quality
> bug here too - I'll file a bug and kfail the remaining failures in a
> separate patch) reason.

Or better yet, use the gnu_inline attribute on those functions.

Thanks,
Andrew Pinski

Re: [patch] explicitly specify -std=gnu89 for gdb.cp/inline-break.exp

[David Blaikie]

[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]

On Fri, Apr 11, 2014 at 7:59 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Fri, Apr 11, 2014 at 4:58 PM, David Blaikie <dblaikie@gmail.com> wrote:
>> This test is intending to use gnu style inline rather than the
>> standard c99 inline semantics. Clang defaults to c99 and the test
>> breaks for this (and other - there's an inlining debug info quality
>> bug here too - I'll file a bug and kfail the remaining failures in a
>> separate patch) reason.
>
> Or better yet, use the gnu_inline attribute on those functions.

Ah, good plan - patch attached for that fix instead.

Though at this point, I'd consider removing the GNUC conditional - for
this test to be meaningful the compiler must support gnu inlining
semantics. Are there compilers that support those semantics but don't
support GCC attribute syntax and the gnu_inline attribute in
particular?

Removing the conditional would cause any compiler that doesn't support
the attributes to just fail to compile, marking the test as untested
rather than producing failures.

[-- Attachment #2: gnu_inline.diff --]
[-- Type: text/plain, Size: 2104 bytes --]

commit a0f7d916bf274325b1535d7f4eade43953cb2bf2
Author: David Blaikie <dblaikie@gmail.com>
Date:   Sun Apr 13 00:01:21 2014 -0700

    Use attribute to specify the required inlining semantics
    
    As suggested by Andrew Pinski.
    
    gdb/testsuite/
    	* gdb.opt/inline-break.c: Fix clang compatibility by specifying
    	gnu_inline semantics via attribute.
    	* gdb.opt/inline-break.exp: Remove -std=c89 now that the test source
    	explicitly specifies the required semantics.

diff --git gdb/testsuite/ChangeLog gdb/testsuite/ChangeLog
index 730c116..44b2eeb 100644
--- gdb/testsuite/ChangeLog
+++ gdb/testsuite/ChangeLog
@@ -6,6 +6,13 @@
 
 2014-04-11  David Blaikie  <dblaikie@gmail.com>
 
+	* gdb.opt/inline-break.c: Fix clang compatibility by specifying
+	gnu_inline semantics via attribute.
+	* gdb.opt/inline-break.exp: Remove -std=c89 now that the test source
+	explicitly specifies the required semantics.
+
+2014-04-11  David Blaikie  <dblaikie@gmail.com>
+
 	* gdb.opt/inline-break.exp: Explicitly specify -std=gnu89 to
 	override Clang's default.
 
diff --git gdb/testsuite/gdb.opt/inline-break.c gdb/testsuite/gdb.opt/inline-break.c
index 9513eec..f8a9ec9 100644
--- gdb/testsuite/gdb.opt/inline-break.c
+++ gdb/testsuite/gdb.opt/inline-break.c
@@ -19,7 +19,7 @@
    this file, and should be regenerated if this file is modified.  */
 
 #ifdef __GNUC__
-# define ATTR __attribute__((always_inline))
+# define ATTR __attribute__((gnu_inline)) __attribute__((always_inline))
 #else
 # define ATTR
 #endif
diff --git gdb/testsuite/gdb.opt/inline-break.exp gdb/testsuite/gdb.opt/inline-break.exp
index 4ff379a..21c958a 100644
--- gdb/testsuite/gdb.opt/inline-break.exp
+++ gdb/testsuite/gdb.opt/inline-break.exp
@@ -19,10 +19,8 @@
 
 standard_testfile
 
-# Explicitly specify gnu89 for gnu inline semantics to override Clang's default
-# of c99.
 if { [prepare_for_testing $testfile.exp $testfile $srcfile \
-          {debug optimize=-O2 additional_flags=-Winline additional_flags=-std=gnu89}] } {
+          {debug optimize=-O2 additional_flags=-Winline}] } {
     return -1
 }
 

Re: [patch] explicitly specify -std=gnu89 for gdb.cp/inline-break.exp

[Pedro Alves]

On 04/13/2014 08:08 AM, David Blaikie wrote:
> On Fri, Apr 11, 2014 at 7:59 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>> On Fri, Apr 11, 2014 at 4:58 PM, David Blaikie <dblaikie@gmail.com> wrote:
>>> This test is intending to use gnu style inline rather than the
>>> standard c99 inline semantics. Clang defaults to c99 and the test
>>> breaks for this (and other - there's an inlining debug info quality
>>> bug here too - I'll file a bug and kfail the remaining failures in a
>>> separate patch) reason.
>>
>> Or better yet, use the gnu_inline attribute on those functions.
> 
> Ah, good plan - patch attached for that fix instead.
> 
> Though at this point, I'd consider removing the GNUC conditional - for
> this test to be meaningful the compiler must support gnu inlining
> semantics. Are there compilers that support those semantics but don't
> support GCC attribute syntax and the gnu_inline attribute in
> particular?

A web search indicates IBM's xlc supports it:

http://pic.dhe.ibm.com/infocenter/zos/v1r13/index.jsp?topic=%2Fcom.ibm.zos.r13.cbclx01%2Ffn_attrib_gnu_inline.htm

(and always_inline too.)

And I know ARM's compiler supports it too.

If there's such a compiler, we can readjust.

The patch is OK.

> Removing the conditional would cause any compiler that doesn't support
> the attributes to just fail to compile, marking the test as untested
> rather than producing failures.

That'd be fine, I think.  In fact, it might even make the test work
under xlc...  (no idea of people actually test that).  That change
is pre-approved (but please do make it a separate change).

Although the test as is requires gnu semantics, we're really after
making sure that setting breakpoints in inline functions work.
We should probably test all off gnu inline semantics, c99
semantics, and also c++ semantics too.

-- 
Pedro Alves

Re: [patch] explicitly specify -std=gnu89 for gdb.cp/inline-break.exp

[Pedro Alves]

On 05/01/2014 11:52 AM, Pedro Alves wrote:

> The patch is OK.

Reviewing the Clang sim/inline patch, I realized this patch hasn't
gone in yet, though it was marked as Accepted in patchwork.

I've pushed it in now.

This is the sort of case the new "Committed" state
will be useful for.

Thanks,
-- 
Pedro Alves