Re: [PATCH v4 00/18] All-stop on top of non-stop

[Luis Machado <lgustavo@codesourcery.com>]

Hi Joel,

On 08/12/2015 03:32 PM, Joel Brobecker wrote:
> Hi Pedro,
>
> On Fri, Aug 07, 2015 at 05:58:37PM +0100, Pedro Alves wrote:
>> On 05/22/2015 12:18 AM, Pedro Alves wrote:
>>> v3: https://sourceware.org/ml/gdb-patches/2015-04/msg00662.html
>>> v2: https://sourceware.org/ml/gdb-patches/2015-04/msg00198.html
>>> v1: https://sourceware.org/ml/gdb-patches/2015-04/msg00073.html
>>>
>>> Compared to v3, a set of minor changes accumulated, to address review
>>> comments and discussions (i.e., no major functional/design changes),
>>> plus fixes to a patch that touched all targets (that I noticed
>>> converted a couple wrong).
>>>
>>> I believe I addressed all review comments thus far.  Barring comments,
>>> I'd like to push this in, and expose it to the build bots.
>>
>> FYI, I pushed this in.  I'm keeping an eye on the build bots.
>>
>> If you spot any related problem, please confirm whether
>> "maint set target-non-stop off" fixes it.
>
> This uncovered what I think is a latent bug in the "how-did-we-never-
> see-this-before" category. Does this patch look OK to you?
>
> gdb/ChangeLog:
>
>          * amd64-tdep.c (amd64_displaced_step_fixup): Fix the mask used to
>          compute RETADDR.
>
> gdb/testsuite/ChangeLog:
>
>          * gdb.base/dso2dso-impl.c, gdb.base/dso2dso-impl.h,
>          gdb.base/dso2dso-pck.c, gdb.base/dso2dso-pck.h, gdb.base/dso2dso.c,
>          gdb.base/dso2dso.exp: New files.
>
> Tested on x86_64-linux, no regression.
>
> Thanks!
>

Two things about the patch. I see that the change to GDB's code is 
almost trivial, but the testcase looks quite involved.

The first thing is that one wouldn't be able to tell what the testcase 
does without looking at the commit log. dso2dso doesn't particularly 
translate to "displaced stepping instruction masking problem on amd64". 
Should we change the testcase name to something a bit more meaningful? 
Maybe document it a bit?

The second point is, should we restrict this testcase to be executed 
only for amd64? Maybe move it to gdb.arch? Unless you actually tried 
this testcase with different architectures and confirmed the testcase is 
sane there, it feels a bit iffy to add/execute this for non-amd64 targets.

In the worst case, we could have a failure due to different instruction 
scheduling schemes (or maybe even a displaced stepping bug) on non-amd64 
targets. At a minimum, we'd have a spurious PASS that wouldn't be 
meaningful for the overall test results since things never failed on 
said architecture in the first place.

Alternatively, for such a trivial fix, shouldn't we go without a testcase?

Luis