Re: [PATCHv2 2/3] gdb: introduce program_space::get_entry_point_info function

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> Hi!
>
> On 2026-06-15 11:29, Andrew Burgess wrote:
>> Pedro Alves <pedro@palves.net> writes:
>> 
>>> On 2026-06-11 22:59, Andrew Burgess wrote:
>>>> +    # Only svr4 targets currently support querying the inferior entry
>>>> +    # address.
>>>> +    if {[istarget *-linux*]} {
>>>
>>> There are more svr4 targets than linux.  I think this style of allow-list has
>>> a good chance of never getting updated.  Deny-lists are better, IMHO.  If the
>>> test fails on some port, that might trigger someone to add the feature
>>> there.
>> 
>> Hey Pedro,
>> 
>> I'm still looking at your other feedback, but before I start making this
>> specific change, I just wanted to clarify how you see this as being
>> different from what I have right now.
>> 
>> If I write this as a deny list, e.g.
>> 
>>  if { ![istarget ....] && ![istarget ....] } {
>>    # Run tests.
>>  }
>> 
>> Is there not the same problem?  We rely on someone realising that the
>> reason the test isn't run on their target is some missing GDB
>> functionality, and them adding the functionality and removing the
>> `istarget` block for their target.
>
>
> Speaking from principles, and not this particular case:
>
> The difference is that there was a failure that made someone see that
> some functionality is missing.  With the allow-list, nobody ever notices it.
>
> With a target-based allow-list, even if someone adds some functionality
> to a port, it's typical to not comb through the testsuite and
> relax the relevant allow-lists to also allow their targets.  
>
> And even if they want to, there's no easy marker to grep for.  E.g, say I
> implement feature X on Windows, then how do I know that I need to
> grep for "istarget Y" to find all the tests that I need to adjust?
> And which Y?  And then which ones that hit my grep should I look at?
>
> I'll give you one example, in gdb.threads/watchpoint-fork.exp:
>
>  # Only GNU/Linux is known to support `set follow-fork-mode child'.
>  if {[istarget "*-*-linux*"]} {
>      test child FOLLOW_CHILD
>  } else {
>      untested "${testfile}: child"
>  }
>
> I think at least FreeBSD has supported that for over a decade.

Sure, but how would rewriting this as you're suggesting have helped in
any way?  From what you suggest below I'm imagining your rewrite of this
would have looked like this:

  if { [supports_fork_follow_child] } {
    test child FOLLOW_CHILD
  } else {
    untested "${testfile}: child"
  }

with:

  proc supports_fork_follow_child {} {
    if { [istarget *-freebsd*] } {
      # Not supported here.  I assume at the point the test was first
      # added this feature wasn't supported on FreeBSD.
      return 0
    }

    # Assume true by default.
    return 1
  }

Now, I agree that this is better as fixing `supports_fork_follow_child`
means we only need to update one proc and all tests that used the proc
would then start testing fork follow child behaviour.  But, as you say,
it's unlikely that when this feature was fixed on FreeBSD the support
proc would actually be updated, so I fail to see how this is
significantly different.

And the example you give is fundamentally different than my code.  What
I wrote is this:

  if { [is_svr4_target] } {
    set expected_result "PATTERN WHEN SUPPORTED"
  } else {
    set expected_result "PATTERN WHEN NOT SUPPORTED"
  }

  gdb_test "some command" $expected_result

The difference here is that if a non-svr4 target is fixed then it will
stop emitting "PATTERN WHEN NOT SUPPORTED" and will start emitting
"PATTERN WHEN SUPPORTED".  If the developer actually runs the complete
testsuite then they will see a PASS -> FAIL for this test, which will
force them to update the `if` condition (but see below).

I tried to investigate if the test you quoted above could be made to
work in the same way; i.e. on the unsupported path, actually run a test
that confirms the feature is unsupported, which would have immediately
identified when the feature started being supported, but I couldn't see
how GDB would fail, most of the fork handling code appears to be
generic, and targets that don't have specific overrides fallback to
process_stratum_target, which doesn't care if we're following the parent
or child.

>
>> 
>> Another option would be for me to just not add any test skipping right
>> now.  I know this will lead to the test failing for some targets as the
>> new feature is only implemented for svr4 targets.  Then, if someone on a
>> failing target cares, they can add the missing feature.  This approach
>> feels a bit mean, I don't like adding failing tests for others, but it
>> does draw attention to the problem.
>
> I think the best would be to just add a reasonable deny-list from
> the get go.  I suspect this is going to be a good enough starting point:
>
> proc supports_process_entry_point {} {
>    if windows || darwin
>      return 0
>
>    if is_svr4_target 
>      return 1 # works even if remote, svr4 is handled on the host.
>
>    if remote
>      return 0 # no RSP packet that gets us the info
>
>    # Assume yes.
>    return 1
> }

I do agree with you that having a 'supports_....' proc will be better
than having to update the `if` condition within the test itself.  If the
supports proc ends up being used multiple times then we only need to
update the one place and all the tests will be fixed.

I'll follow the structure you propose here as I'd like to progress this
patch.  I'll post a v4 with the update soon.

Thanks,
Andrew