Re: [PATCH v2 01/13] gdb/linux-record: Add testcases for a few syscalls.

[Pedro Alves <palves@redhat.com>]

On 10/29/2015 12:31 PM, Marcin Kościelnicki wrote:
> On 29/10/15 12:30, Pedro Alves wrote:
>> On 10/22/2015 02:39 PM, Marcin Kościelnicki wrote:

>> I ran your tests against pristine/unfixed gdb, and linux-readv-reverse.exp
>> passes cleanly.  This is on x86_64.  Is that expected?  The comments in
>> the test seem to suggest it would fail.
> 
> That's expected.  The test would only fail for x32 ABI, since it had the 
> wrong size_iov, size_pointer, size_size_t.

Ah.

>>> +#define _GNU_SOURCE
>>> +#include <unistd.h>
>>> +#include <fcntl.h>
>>> +#include <sys/stat.h>
>>> +
>>> +void marker1 ()
>>
>> We follow GNU formatting in tests too, unless there's a good
>> reason not to.  So, line break after void.  Also, this is C,
>> so write "(void)" for parameters.  Thus:
>>
>> void
>> marker1 (void)
>> {
> 
> OK.
>>
>>> +{
>>> +}
>>> +
>>> +void marker2 ()
>>
>> Likewise.
>>
>>> +{
>>> +}
>>> +
>>> +struct stat buf;
>>> +
>>> +int main() {
>>
>> int
>> main ()
>> {
>>
> Not int main(void)?

Oh, yes, (void).  Sorry, I only payed attention to the
formatting and missed that.

>> ... I think most of these tests could/should drop this target check,
>> and drop the "linux-" in the filename as well.  Any target/port
>> that supports record/reverse execution should be able to run
>> this, as long as it manages to compile the test program.  And if the
>> test program doesn't compile in such a port, it'll be automatically
>> skipped.
> 
> OK.  So a testcase that doesn't compile is not a problem?  Because I 
> just noticed the time test doesn't compile on platforms that don't have 
> a time syscall, like s390x...

It just results in the test being marked UNTESTED.

It's usually better to skip a test on targets that are known
not to support something than the other way around.  Like:

 if [istarget "*s390*"] then {
     ...
     return
 }

Because otherwise, new ports (or when features are added to
existing ports) end up not enabling the tests that might
run there.

Thanks,
Pedro Alves