On Tue, May 27, 2014 at 3:33 AM, Richard Biener
<richard.guent...@gmail.com> wrote:
> On Mon, May 26, 2014 at 9:01 PM, Patrick Palka <patr...@parcs.ath.cx> wrote:
>> Hi,
>>
>> This patch teaches the C++ frontend to warn on NULL checks against
>> inline functions and it teaches the middle-end to fold NULL checks
>> against inline functions. These two things are currently done for
>> non-inline C++ functions, but inline functions are exceptional in that
>> the C++ frontend marks them as weak, and NULL checks against weak
>> symbols cannot be folded in general because the symbol may be mapped to
>> NULL at link-time.
>>
>> But in the case of an inline function, the fact that it's a weak symbol
>> is an implementation detail. And because it is not permitted to
>> explicitly give an inline function the "weak" attribute (see
>> handle_weak_attribute), in order to acheive $SUBJECT it suffices to
>> assume that all inline functions are non-null, which is what this patch
>> does.
>>
>> Bootstrap and regtest against x86_64-unknown-linux-gnu in progress. Is
>> this patch OK assuming no regressions?
>
> What about always_inline function wrappers as used in FORTIFY_SOURCE?
> IIRC the actual referenced function may be declared weak and thus the
> address can compare to NULL? Sth like
>
> extern void __foo (void) __attribute__((weak,asm("foo")));
> extern inline __attribute__((always_inline,gnu_inline)) void foo (void)
> {
> __foo ();
> }
>
> int main()
> {
> if (foo == 0)
> return 0;
> abort ();
> }
>
> The issue is that the inline and alias may be hidden to the user and thus
> he'll get unwanted and confusing warnings.
>
> Richard.
So as it stands the foo == 0 check in the above example gets folded
with or without my patch. The issue here seems to be related to the
use of gnu_inline. The address of an inline function marked
gnu_inline shouldn't be considered non-NULL because a standalone
function does not actually get emitted. Of course, one could inspect
aliases but in general it is not correct to assume such functions are
non-NULL. Does this sound right?
This issue has slight overlap with the issue that my patch tries to
fix. I could try to handle it as well.
