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

Pedro Alves
2015-10-30T10:36:36Z

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