Re: gdb/linux-record fixes

["Marcin Kościelnicki" <koriakin@0x04.net>]

On 20/10/15 13:07, Pedro Alves wrote:
> On 10/19/2015 06:17 PM, Marcin Kościelnicki wrote:
>
>> Yeah, they're not covered by the testsuite.  Actually, there seem to be
>> only two tests in gdb.reverse suite that even touch syscalls:
>> sigall-reverse (signal, sigprocmask, exit_group) and watch-reverse
>> (read, write).  No wonder that syscall handling is buggy...
>>
>> Stepping forward and backward over pipe/time/waitpid would indeed do the
>> trick for patch #6.
>
> Can I convince you to add that to the patch (and likewise to others that
> might not be overly hard)?

I'll do that, if I'm not overcome by dejaGNU...  I have no idea how that 
stuff works at the moment.

> BTW, you'll also need to include ChangeLog entries.  Please check here:
>
>    https://sourceware.org/gdb/wiki/ContributionChecklist

OK, will do.

>> Some others are trickier to test, though - for
>> example to test #3 (overlong sigaction/sigset_t) you'd need to manually
>> invoke the actual linux syscalls, taking care to pull the right struct
>> definitions from kernel headers, and make sure they're located near the
>> end of segment bounduary (so that accessing after the end causes an
>> error) - the glibc wrappers will always allocate relevant structs on
>> stack, which almost certainly has extra bytes after the struct, hiding
>> the bug.
>
> OOC, how did you first notice the bug?

I'm working on s390 support, and noticed a lot of weirdness when filling 
the size_* fields for the new target and comparing with others.  I dug 
down into syscall handling code and noticed a lot more problems.  I also 
stepped through getgroups syscall on i386 to make sure size mismatches 
really cause problems (getgroups was recorded wrong because of the gid 
size issue).

>>
>> BTW, I was thinking of a self-testing approach for linux-record:
>>
>> - if gdb debugging is enabled, record changes on each instruction twice:
>> before and after execution
>> - memory/register contents recorded in "before" stage should match
>> contents previously recorded in "after" stage, if any (ie. if it's not
>> the first time we see a register or a memory byte).  If there's a
>> mismatch, record missed something.
>>
>> Unfortunately, that's a non-starter for multithreaded programs, or for
>> progams involving shared memory :(
>
> Sounds like a good idea.  I don't see multithreading being an issue,
> as target record doesn't really work with multithreaded programs to begin
> with.  Also, I don't think we'd want it behind the usual "set debug record"
> knob, as that sounds like it could actually change the record target's
> behavior, and heisenbugs.  Probably put it under its own
> "maint set record self-check on" or some such knob.
>
> This reminds me that Yao once wrote a test that did something like
> that (registers only, no gdb magic):
>
>    https://sourceware.org/ml/gdb-patches/2015-05/msg00482.html
>
> Not sure what happened to that.
>
> Thanks,
> Pedro Alves
>