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~
