On 3/14/25 13:59, Richard Henderson wrote:
> On 3/14/25 13:34, Pierrick Bouvier wrote:
>> On 3/14/25 13:03, Richard Henderson wrote:
>>> I'm not quite sure what you're arguing for here.
>>> A build-time error is vastly preferable to a run-time error.
>>>
>>
>> Even though this specific patch is safe (code calling those functions should be under
>> system anyway, so it should not be linked in a user binary), I just wonder if we should
>> not add explicit checks for this, for other kind of replacement we'll have to do.
>
> Any such runtime check should not be for "system" vs "user", but for "feature enabled".
>
From our build system point of view, I don't really see what is the
difference. That's part of why I insisted previously on cleaning user vs
system ifdefs at the same time we make files common, but the answer I
had was "We don't need to do that now", so I stopped arguing.
When building user vs system, we use ifdef to detect this case, and we
select different implementations and include a specific set of source
files, the same way we do for some features, or specific targets. So,
why should it be treated differently, or later?
>>> If it's a lesser used configuration or feature, a run-time error could lay dormant for
>>> years before a user encounters it.
>>>
>>
>> Sure, but wouldn't it better to have an explicit assert, instead of observing a random
>> behaviour?
>
> What random behaviour are you suggesting?
>
In case we return a stub value by accident, when you expect to have a
side effect done somewhere. And that it would not result in an immediate
crash, but later at a random place.
Maybe the case just does not exist, but my point was that it does not
cost anything to add an assert just in case.
>> I'm just worried we end up calling something we should not (user vs system, or any other
>> ifdef CONFIG/TARGET that might be hidden somewhere), and silently return a wrong value,
>> which would not be covered by our test suite.
>
> Where is the wrong value going to be returned from, the stub?
> Yes, many stubs do abort, typically after the "enabled" stub returns false.
>
Many, but not all of them I guess.
> It's still best if "feature enabled" is compile-time false when possible, such that
> everything after the feature check gets compiled out. At which point we don't need the
> stubs: they won't be reachable and errors are caught at build-time.
>
Sure, but as we saw, it's not always possible, because some calls are
under: if (X_enabled()) {...do_X()...}, which requires to be able to
link do_X even though it will be dead code at runtime.
Maybe my concern is unfounded, but seeing some of the inline stubs and
the ifdefs associated, I hope we won't miss a corner case.
>
> r~