Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

[Martin Sebor <msebor at gmail dot com>]

> > > I suspect the back end or even the middle end route isn't going to
> > > work even if there was enough context to diagnose the problem
> > > expressions because some of them will have been optimized away by then
> > > (e.g., 'if (& __builtin_foo != 0)' is optimized into if (1) by gimple).
> >
> > I was thinking that if they're optimized away, they aren't problematic
> > anymore; that was part of the attraction for me of handling this lower
> > down.
>
> Yes, they're not problematic in that they don't cause linker unsats.
> I have a few concerns with the approach of accepting or rejecting
> constructs based on optimization (e.g., that it might lead to
> similar problems as with -Wmaybe-uninitialized; or that it could
> be perceived as inconsistent). But it may not be as bad as it seems
> -- let me look into it if only as a learning exercise and see how
> it goes.

I've prototyped this approach in a couple places in the middle
end. Both implementations are very simple and work great when
the code isn't optimized away. The problem is that the
optimizations done at various points between the front end and
the final gimplification make the diagnostics inconsistent.

For instance, with F being the type of the builtin, this is
diagnosed in the first prototype:

  F* foo () { if (0) return __builtin_trap; return 0; }

but this isn't:

  F* bar () { return 0 ? __builtin_trap : 0; }

because the ternary ?: expression is folded into a constant by
the front end even before it reaches the gimplifier, while the
if-else statement isn't folded until the control flow graph is
built. (As an aside: I'm wondering why that is. Why have the
front end do any optimization at all if the middle can and
does them too, and better?)

Diagnosing neither of these cases might perhaps seem like the
way to go since they are both safe (don't result in a linker
error). I implemented this approach in the second prototype in
a later pass but that exposed even more inconsistencies as even
more code is optimized (for instance, exception handling).

I think this sort of inconsistency in diagnosing semantically
equivalent language constructs would be viewed as arbitrary
and be unhelpful to users. It's too bad because otherwise this
solution would have been quite elegant and simple (touching
just one function).

Unless you have some other ideas I'm going to abandon this
approach see if the original patch can be simplified by
consoldating some of the handling into mark_rvalue_use.

Martin

PS Another problem, one that in my view sinks the middle end
approach, is the fact that references in unused static inline
functions aren't diagnosed either, even if calling the inline
function would result in taking the address of the builtin
function. E.g., this isn't diagnosed:

  static inline F* baz (void) { return __builtin_trap; }

unless baz() is called. This would make the feature largely
ineffective in C++ template libraries.