Build gdb.opt/inline-*.exp tests at -O0, rely on __attribute__((always_inline)) (was: Re: [PATCH v3 24/34] Push thread->control.command_interp to the struct thread_fsm)
Commit Message
On 07/01/2016 12:02 PM, Thomas Preudhomme wrote:
> The new tests added by this patch fail for arm-none-eabi targets because -O2
> leads to instructions to be reordered widely. In this case, the instruction
> that follows the first one for line 64 is related to line 70 so the test is
> failing. Shouldn't this test be compiled with -Og and probably also -finline-
> small-functions -findirect-inlining -fpartial-inlining which relates to
> inlining and are included in -O2.
Or even just plain -O0. See the commit log below. WDYT?
--------------
Subject: [PATCH] Build gdb.opt/inline-*.exp tests at -O0, rely on
__attribute__((always_inline))
A test recently added to gdb.opt/inline-cmds.exp fails for
arm-none-eabi targets because -O2 leads to instructions to be
reordered widely.
I guess it might have made sense years ago to enable optimization in
these tests, but I fail to see the need for that nowadays.
Using -O0 while relying on __attribute__((always_inline)), which is
already used in the tests [1] [2], avoids this sort of trouble, while
still exercising the inlining-related use cases that are the focus of
these tests.
I think that nowadays we can safely assume that all compilers we care
about support __attribute__((always_inline)) or similar.
[1] - Except one spot that missed it.
[2] - Note that the .exp files make sure the frames that should have
been inlined are indeed inlined, with "info frame".
gdb/testsuite/ChangeLog:
2016-07-01 Pedro Alves <palves@redhat.com>
* gdb.opt/inline-break.exp: Remove optimize=-O2.
* gdb.opt/inline-bt.exp: Likewise.
* gdb.opt/inline-cmds.exp: Remove optimize=-O2 and add
additional_flags=-Winline.
* gdb.opt/inline-locals.exp: Likewise.
* gdb.opt/inline-markers.c (ATTR): Define.
(inlined_fn): Use it.
---
gdb/testsuite/gdb.opt/inline-break.exp | 2 +-
gdb/testsuite/gdb.opt/inline-bt.exp | 2 +-
gdb/testsuite/gdb.opt/inline-cmds.exp | 2 +-
gdb/testsuite/gdb.opt/inline-locals.exp | 2 +-
gdb/testsuite/gdb.opt/inline-markers.c | 8 +++++++-
5 files changed, 11 insertions(+), 5 deletions(-)
Comments
On Friday 01 July 2016 13:05:35 Pedro Alves wrote:
> On 07/01/2016 12:02 PM, Thomas Preudhomme wrote:
> > The new tests added by this patch fail for arm-none-eabi targets because
> > -O2 leads to instructions to be reordered widely. In this case, the
> > instruction that follows the first one for line 64 is related to line 70
> > so the test is failing. Shouldn't this test be compiled with -Og and
> > probably also -finline- small-functions -findirect-inlining
> > -fpartial-inlining which relates to inlining and are included in -O2.
>
> Or even just plain -O0. See the commit log below. WDYT?
I'm not very familiar with GDB but the description looks great indeed. Thanks!
Best regards,
Thomas
>
> --------------
> Subject: [PATCH] Build gdb.opt/inline-*.exp tests at -O0, rely on
> __attribute__((always_inline))
>
> A test recently added to gdb.opt/inline-cmds.exp fails for
> arm-none-eabi targets because -O2 leads to instructions to be
> reordered widely.
>
> I guess it might have made sense years ago to enable optimization in
> these tests, but I fail to see the need for that nowadays.
>
> Using -O0 while relying on __attribute__((always_inline)), which is
> already used in the tests [1] [2], avoids this sort of trouble, while
> still exercising the inlining-related use cases that are the focus of
> these tests.
>
> I think that nowadays we can safely assume that all compilers we care
> about support __attribute__((always_inline)) or similar.
>
> [1] - Except one spot that missed it.
>
> [2] - Note that the .exp files make sure the frames that should have
> been inlined are indeed inlined, with "info frame".
>
> gdb/testsuite/ChangeLog:
> 2016-07-01 Pedro Alves <palves@redhat.com>
>
> * gdb.opt/inline-break.exp: Remove optimize=-O2.
> * gdb.opt/inline-bt.exp: Likewise.
> * gdb.opt/inline-cmds.exp: Remove optimize=-O2 and add
> additional_flags=-Winline.
> * gdb.opt/inline-locals.exp: Likewise.
> * gdb.opt/inline-markers.c (ATTR): Define.
> (inlined_fn): Use it.
> ---
> gdb/testsuite/gdb.opt/inline-break.exp | 2 +-
> gdb/testsuite/gdb.opt/inline-bt.exp | 2 +-
> gdb/testsuite/gdb.opt/inline-cmds.exp | 2 +-
> gdb/testsuite/gdb.opt/inline-locals.exp | 2 +-
> gdb/testsuite/gdb.opt/inline-markers.c | 8 +++++++-
> 5 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.opt/inline-break.exp
> b/gdb/testsuite/gdb.opt/inline-break.exp index b2aa22e..ac56b04 100644
> --- a/gdb/testsuite/gdb.opt/inline-break.exp
> +++ b/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 additional_flags=-Winline}] } {
> return -1
> }
>
> diff --git a/gdb/testsuite/gdb.opt/inline-bt.exp
> b/gdb/testsuite/gdb.opt/inline-bt.exp index 63d76e2..13c6993 100644
> --- a/gdb/testsuite/gdb.opt/inline-bt.exp
> +++ b/gdb/testsuite/gdb.opt/inline-bt.exp
> @@ -17,7 +17,7 @@ standard_testfile .c inline-markers.c
>
> if {[prepare_for_testing $testfile.exp $testfile \
> [list $srcfile $srcfile2] \
> - {debug optimize=-O2 additional_flags=-Winline}]} {
> + {debug additional_flags=-Winline}]} {
> return -1
> }
>
> diff --git a/gdb/testsuite/gdb.opt/inline-cmds.exp
> b/gdb/testsuite/gdb.opt/inline-cmds.exp index 684f4dd..6c84848 100644
> --- a/gdb/testsuite/gdb.opt/inline-cmds.exp
> +++ b/gdb/testsuite/gdb.opt/inline-cmds.exp
> @@ -19,7 +19,7 @@ set MIFLAGS "-i=mi"
> standard_testfile .c inline-markers.c
>
> if {[prepare_for_testing $testfile.exp $testfile \
> - [list $srcfile $srcfile2] {debug optimize=-O2}]} {
> + [list $srcfile $srcfile2] {debug additional_flags=-Winline}]} {
> return -1
> }
>
> diff --git a/gdb/testsuite/gdb.opt/inline-locals.exp
> b/gdb/testsuite/gdb.opt/inline-locals.exp index df2253a..36f7ed2 100644
> --- a/gdb/testsuite/gdb.opt/inline-locals.exp
> +++ b/gdb/testsuite/gdb.opt/inline-locals.exp
> @@ -16,7 +16,7 @@
> standard_testfile .c inline-markers.c
>
> if {[prepare_for_testing $testfile.exp $testfile \
> - [list $srcfile $srcfile2] {debug optimize=-O2}]} {
> + [list $srcfile $srcfile2] {debug additional_flags=-Winline}]} {
> return -1
> }
>
> diff --git a/gdb/testsuite/gdb.opt/inline-markers.c
> b/gdb/testsuite/gdb.opt/inline-markers.c index cf92e79..41f8a38 100644
> --- a/gdb/testsuite/gdb.opt/inline-markers.c
> +++ b/gdb/testsuite/gdb.opt/inline-markers.c
> @@ -13,6 +13,12 @@
> You should have received a copy of the GNU General Public License
> along with this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
> +#ifdef __GNUC__
> +# define ATTR __attribute__((always_inline))
> +#else
> +# define ATTR
> +#endif
> +
> extern int x, y;
> extern volatile int z;
>
> @@ -26,7 +32,7 @@ void marker(void)
> x += y - z; /* set breakpoint 2 here */
> }
>
> -inline void inlined_fn(void)
> +inline ATTR void inlined_fn(void)
> {
> x += y + z;
> }
On Friday 01 July 2016 16:24:38 Thomas Preudhomme wrote:
> On Friday 01 July 2016 13:05:35 Pedro Alves wrote:
> > On 07/01/2016 12:02 PM, Thomas Preudhomme wrote:
> > > The new tests added by this patch fail for arm-none-eabi targets because
> > > -O2 leads to instructions to be reordered widely. In this case, the
> > > instruction that follows the first one for line 64 is related to line 70
> > > so the test is failing. Shouldn't this test be compiled with -Og and
> > > probably also -finline- small-functions -findirect-inlining
> > > -fpartial-inlining which relates to inlining and are included in -O2.
> >
> > Or even just plain -O0. See the commit log below. WDYT?
>
> I'm not very familiar with GDB but the description looks great indeed.
> Thanks!
Hi Pedro,
What is the status of that patch?
Best regards,
Thomas
On 07/15/2016 01:05 PM, Thomas Preudhomme wrote:
> Hi Pedro,
>
> What is the status of that patch?
Sorry, I've been away for a couple weeks.
Seems like no one objected, so I'm putting it in now.
Thanks,
Pedro Alves
On Tuesday 19 July 2016 18:02:37 Pedro Alves wrote:
> On 07/15/2016 01:05 PM, Thomas Preudhomme wrote:
> > Hi Pedro,
> >
> > What is the status of that patch?
>
> Sorry, I've been away for a couple weeks.
>
> Seems like no one objected, so I'm putting it in now.
Test is now PASSing for us. Thanks!
Best regards,
Thomas
@@ -20,7 +20,7 @@
standard_testfile
if { [prepare_for_testing $testfile.exp $testfile $srcfile \
- {debug optimize=-O2 additional_flags=-Winline}] } {
+ {debug additional_flags=-Winline}] } {
return -1
}
@@ -17,7 +17,7 @@ standard_testfile .c inline-markers.c
if {[prepare_for_testing $testfile.exp $testfile \
[list $srcfile $srcfile2] \
- {debug optimize=-O2 additional_flags=-Winline}]} {
+ {debug additional_flags=-Winline}]} {
return -1
}
@@ -19,7 +19,7 @@ set MIFLAGS "-i=mi"
standard_testfile .c inline-markers.c
if {[prepare_for_testing $testfile.exp $testfile \
- [list $srcfile $srcfile2] {debug optimize=-O2}]} {
+ [list $srcfile $srcfile2] {debug additional_flags=-Winline}]} {
return -1
}
@@ -16,7 +16,7 @@
standard_testfile .c inline-markers.c
if {[prepare_for_testing $testfile.exp $testfile \
- [list $srcfile $srcfile2] {debug optimize=-O2}]} {
+ [list $srcfile $srcfile2] {debug additional_flags=-Winline}]} {
return -1
}
@@ -13,6 +13,12 @@
You should have received a copy of the GNU General Public License
along with this program. If not, see <http://www.gnu.org/licenses/>. */
+#ifdef __GNUC__
+# define ATTR __attribute__((always_inline))
+#else
+# define ATTR
+#endif
+
extern int x, y;
extern volatile int z;
@@ -26,7 +32,7 @@ void marker(void)
x += y - z; /* set breakpoint 2 here */
}
-inline void inlined_fn(void)
+inline ATTR void inlined_fn(void)
{
x += y + z;
}