Re: [PATCH 09/12] gdb_target_is_native -> gdb_protocol_is_native

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

On 5/9/24 17:49, Pedro Alves wrote:
> On 2024-05-09 16:01, Bernd Edlinger wrote:
>> On 5/9/24 15:31, Pedro Alves wrote:
>>>
>>> On 2024-05-09 14:19, Bernd Edlinger wrote:
>>>> Hmm, okay, it is better than now, 
>>>
>>> Indeed, seems strictly better.  Some tests that are meant for the native target are now properly
>>> skipped.
>>>
>>>> but the test casese that are affected, would probably
>>>> be broken by this change, if my target toolchain would either have the -linux in the name,
>>>> or the newlib would have a sleep and/or support the #include <sys/mman.h>.
>>>
>>> I am afraid I don't know what you mean by this.  Why would they be broken?  Can you clarify?
>>
>> What I mean is this:
>>
>> there are in total 44 test cases failed to compile because of undefined reference to sleep, usleep or nanosleep
>> with your patch it is now one less.
>>
>> But it would be piece of cake to implement some functions like
>> sleep(), usleep(), and nanosleep in the newlib, and then make the simulator simulate it.
>>
>> Likewise there are currently 5 test cases failed to compile because of missing sys/mman.h header,
>> and with your patch it will be one less, but what if we want to add that to the
>> simulator, why cant we simulate a linux O/S?
>>
>> What is so special in the one test case that changed the behavior, that
>> explains why the other 4 are good enough to try to include, and see if <sys/mman.h> works?
>>
> 
> The gdb.base/load-command.exp testcase just doesn't make sense to test with the native
> target, since with the native target, you don't use the "load" command to load programs.
> So the testcase checks for that:
> 
> if [gdb_protocol_is_native] {
>     unsupported "the native target does not support the load command"
>     return
> }
> 
> Normally, a testcase that checks whether it is testing with the native target does so because it is
> testing some feature of the native target (the ptrace support, etc.), and so such an early check
> isn't that much about whether sleep or sys/mman.h exists (they don't exist on native Windows,
> for example, and there are native-only tests that you'd still want to run there).
> 
> If a testcase should run against the sim target and would no longer run because we set
> gdb_protocol, then that would be a problem with the testcase itself, for using the wrong
> predicate to skip itself.  We've had plenty of cases of testcases using the wrong
> predicate over the years, that is something that we're fixing pretty frequently, so
> please don't be surprised if we need to do that after this change too.
> 
> Looking at gdb.base/many-headers.exp for example (the first in your previous message
> that shows compile errors), it has:
> 
> if { [target_info gdb_protocol] != "" } {
>     # Even though the feature under features being tested are supported by
>     # gdbserver, the way this test is written doesn't make it easy with a
>     # remote target.
>     unsupported "not native"
>     return
> }
> 
> Note the comment -- it is saying to skip testing on remote, but then it skips
> on any board that sets gdb_protocol to anything (meaning, not native).
> If this truly should only be skipped with remote targets, then it should instead
> do this:
> 
> require gdb_protocol_is_remote
> 
> which is equivalent to:
> 
>   if { [target_info gdb_protocol] == "remote" || [target_info gdb_protocol] == "extended-remote" } {
>      return
>   }
> 
> But, actually looking at the testcase, we see that it launches a program outside gdb, so that
> the program crashes and generates a core (core_find), and then debugs the core under gdb,
> with GDB stack limited to 4MB.  From the commit's log (31aceee86308), it's the equivalent
> of doing:
> 
>     $ ulimit -s 4096
>     $ gdb -batch ./a.out core.saved
>     [New LWP 19379]
>     Segmentation fault (core dumped)
>     ...
> 
> So, I don't really understand what the "features being tested are supported by gdbserver"
> remark is alluding to, since gdbserver does not support loading cores.  But then again,
> the test doesn't even connect to the native target nor the remote target anyhow.  So
> we could let the testcase run when testing against a gdbserver board anyhow.
> 
> Maybe what the comment is trying to say is, not easy to test with a "remote target"
> in dejagnu sense, i.e., when you have to connect to a remote host board running on a
> separate system, as in that scenario it's not easy to set ulimit.  Is so, a better
> predicate to check would be "require !is_remote host" or "require isnative".
> 
> A similar analysis would have to be done to the other testcases.

Ah, okay, thanks for the explanation.

I was probably confused that sim looks an feels like a kind of native target,
but it supports the load command, and the test cases can read and write
local files, etc.  Therefore it is of course easy to fix things in the wrong way.
So I agree with your patch, and that any fallout can be fixed later
in the test cases when necessary.


Thanks
Bernd.