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

[Pedro Alves <pedro@palves.net>]

On 2026-06-23 10:46, Andrew Burgess wrote:
> 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.

Well, with the proc, you effectively turned:

  if {[istarget "*-*-linux*"]} {
      test child FOLLOW_CHILD
  } else {
      untested "${testfile}: child"
  }

into:

  if {![istarget "*-*-freebsd*"]} {
      test child FOLLOW_CHILD
  } else {
      untested "${testfile}: child"
  }

... meaning, any new target that comes along, after the testcase is written,
automatically gets that test exercised.  While the "before" is stuck in testing
only on linux and nobody ever remembers to update it.  That's the main point
about allow vs deny lists.

Also, the wrapper proc is a lot more discoverable, especially if its in
gdb.exp.  You only have to discover it once, and from there you can easily
find all the testcases that use it.  With explicit "istarget", you have the
issue I mentioned earlier: no clue what to look for.  Even assuming you
ran into that one in gdb.threads/watchpoint-fork.exp one, which other testcases
have a similar issue?  Maybe there are others using "istarget linux"?  Or some
other slightly different if condition?

> 
> 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

OK, I missed that.  That's even better, and takes care of my main
concern -- discoverability.

> 
> 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).

Agreed, and that's the ideal.  Thanks.

Pedro Alves

> 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.