Re: [PATCH] Fix gdb.base/step-indirect-call-thunk.exp

[Carl Love <cel@us.ibm.com>]

Bruno:

Thanks for the feedback.  I updated the commit message and will send
out version 2 of the patch.

                     Carl Love

On Fri, 2022-07-01 at 09:37 -0300, Bruno Larsen wrote:
> On 6/15/22 13:21, Carl Love via Gdb-patches wrote:
> > GDB maintainers:
> > 
> > The gdb regression test gdb.base/step-indirect-call-thunk.exp
> > currently
> > does not run on X86 due to a compile error related to incompatible
> > gcc
> > command line argument.  Secondly, the gcc command line arguments
> > that
> > are used are specific to Intel thus generating an unsupported
> > command
> > line error when compiled on other architectures.
> > 
> > This patch fixes the command line arguments so the test will
> > compile on
> > X86.  It also adds a check so the test will only run on X86.
> > 
> > Please let me know if this patch is acceptable for mainline.
> > 
> >                             Carl Love
> > 
> 
> Hi Carl!
> 
> Thanks for looking at this. The code part of the patch looks good,
> but I'd suggest a bit
> of a change to the commit message.
> 
> 
> > --------------------------------------------------------------
> > Fix gdb.base/step-indirect-call-thunk.exp
> > 
> > This test fails on Intel X86-64 with the error:
> > 
> > Executing on host: gcc  -fno-stack-protector  -fdiagnostics-
> > color=never
> > -mindirect-branch=thunk -mfunction-return=thunk -c -g
> > -o /.../gdb/testsuite/outputs/gdb.base/step-indirect-call-
> > thunk/step-indirect-call-thunk0.o
> > /.../gdb/testsuite/gdb.base/step-indirect-call-thunk.c
> > (timeout = 300) builtin_spawn -ignore SIGHUP gcc -fno-stack-
> > protector
> > -fdiagnostics-color=never -mindirect-branch=thunk -mfunction-
> > return=thunk -c
> > -g -o /.../gdb/testsuite/outputs/gdb.base/step-indirect-call-
> > thunk/step-indirect-call-thunk0.o
> > /.../binutils-gdb-current/gdb/testsuite/gdb.base/step-indirect-
> > call-thunk.c
> > /.../gdb/testsuite/gdb.base/step-indirect-call-thunk.c:
> >   In function 'inc': /.../gdb/testsuite/gdb.base/step-indirect-
> > call-thunk.c:
> > 22:1: error: '-mindirect-branch' and '-fcf-protection' are not
> > compatible
> >     22 | {                /* inc.1 */
> > 
> > As stated in the error message the default "-fcf-protection" and
> > "-mindirect-branch' are in compatible.  The fcf-protection argument
> > needs to be
> > "-fcf-protection=none" for the test to compile on Intel.
> 
> This problem doesn't happen on my machine, which is using gcc-8.5.0;
> I think the default
> -fcf-protection value was changed somewhere between gcc-8.5.0 and the
> one you're using.
> I'd mention something along these lines:
> 
> 	"Due to changes in the default value of -fcf-protection on
> newer gccs, the test ... can fail."
> 
> Also, remember that the commit message should assume the title of the
> commit was not read,
> so please use the full test name instead of "This test".
> 
> 
> > The test also fails on PowerPC as the "-mindirect-branch' is an
> > Intel specific
> > GCC command line argument.  A check for X86 is added so the test
> > will only run
> > on X86 platforms.
> 
> I'd remove the specific mention to PowerPC and just say that
> -mindirect-branch is x86
> specific.
> 
> With these, I'd give an OK to this patch, but I can't approve it for
> pushing.
> 
> Cheers!
> Bruno Larsen
> > The patch has been tested and verified on Power 10 and Intel X86-64 
> > systems with
> > no regressions.
> > ---
> >   gdb/testsuite/gdb.base/step-indirect-call-thunk.exp | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gdb/testsuite/gdb.base/step-indirect-call-thunk.exp
> > b/gdb/testsuite/gdb.base/step-indirect-call-thunk.exp
> > index 761e1d9a280..7c1b53c99be 100644
> > --- a/gdb/testsuite/gdb.base/step-indirect-call-thunk.exp
> > +++ b/gdb/testsuite/gdb.base/step-indirect-call-thunk.exp
> > @@ -15,7 +15,11 @@
> >   
> >   standard_testfile
> >   
> > -set cflags "-mindirect-branch=thunk -mfunction-return=thunk"
> > +if { ![istarget "x86*"] } {
> > +    return
> > +}
> > +
> > +set cflags "-mindirect-branch=thunk -mfunction-return=thunk -fcf-
> > protection=none"
> >   if { [prepare_for_testing "failed to prepare" $testfile $srcfile
> > \
> >           [list debug "additional_flags=$cflags"]] } {
> >       return -1