| Message ID | 1401130917-4121-1-git-send-email-patrick@parcs.ath.cx |
|---|---|
| State | New |
| Headers | show |
On Mon, 26 May 2014, Patrick Palka wrote: > 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. Thanks for working on this. You may want to mention PR 59948 in the ChangeLog. In the comments of that PR, Honza seems to have a different idea of what the fold-const test should look like.
On Mon, May 26, 2014 at 4:26 PM, Marc Glisse <marc.glisse@inria.fr> wrote: > On Mon, 26 May 2014, Patrick Palka wrote: > >> 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. > > > Thanks for working on this. You may want to mention PR 59948 in the > ChangeLog. In the comments of that PR, Honza seems to have a different idea > of what the fold-const test should look like. I wonder, then, if the test in c-common.c is also broken... Thanks for the heads up about the PR.
On Mon, 26 May 2014, Patrick Palka wrote: > On Mon, May 26, 2014 at 4:26 PM, Marc Glisse <marc.glisse@inria.fr> wrote: >> On Mon, 26 May 2014, Patrick Palka wrote: >> >>> 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. >> >> >> Thanks for working on this. You may want to mention PR 59948 in the >> ChangeLog. In the comments of that PR, Honza seems to have a different idea >> of what the fold-const test should look like. > > I wonder, then, if the test in c-common.c is also broken... Thanks > for the heads up about the PR. I think I should also mention PR 61144 which says the test can be wrong in the other direction (I didn't look at it closely). But even if the tests are broken, your patch may still be an improvement while waiting for the rewrite...
On Mon, May 26, 2014 at 9:01 PM, Patrick Palka <patrick@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. > 2014-05-26 Patrick Palka <patrick@parcs.ath.cx> > > * c-family/c-common.c (decl_with_nonnull_addr_p): Assume inline > functions are non-null. > * fold-const.c (tree_single_nonzero_warnv_p): Likewise. > --- > gcc/c-family/c-common.c | 4 +++- > gcc/fold-const.c | 5 ++++- > gcc/testsuite/g++.dg/inline-1.C | 14 ++++++++++++++ > 3 files changed, 21 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/inline-1.C > > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c > index 6ec14fc..d4747a0 100644 > --- a/gcc/c-family/c-common.c > +++ b/gcc/c-family/c-common.c > @@ -4508,7 +4508,9 @@ decl_with_nonnull_addr_p (const_tree expr) > return (DECL_P (expr) > && (TREE_CODE (expr) == PARM_DECL > || TREE_CODE (expr) == LABEL_DECL > - || !DECL_WEAK (expr))); > + || !DECL_WEAK (expr) > + || (TREE_CODE (expr) == FUNCTION_DECL > + && DECL_DECLARED_INLINE_P (expr)))); > } > > /* Prepare expr to be an argument of a TRUTH_NOT_EXPR, > diff --git a/gcc/fold-const.c b/gcc/fold-const.c > index 188b179..2796079 100644 > --- a/gcc/fold-const.c > +++ b/gcc/fold-const.c > @@ -16052,7 +16052,10 @@ tree_single_nonzero_warnv_p (tree t, bool *strict_overflow_p) > || (DECL_CONTEXT (base) > && TREE_CODE (DECL_CONTEXT (base)) == FUNCTION_DECL > && auto_var_in_fn_p (base, DECL_CONTEXT (base))))) > - return !VAR_OR_FUNCTION_DECL_P (base) || !DECL_WEAK (base); > + return !VAR_OR_FUNCTION_DECL_P (base) > + || !DECL_WEAK (base) > + || (TREE_CODE (base) == FUNCTION_DECL > + && DECL_DECLARED_INLINE_P (base)); > > /* Constants are never weak. */ > if (CONSTANT_CLASS_P (base)) > diff --git a/gcc/testsuite/g++.dg/inline-1.C b/gcc/testsuite/g++.dg/inline-1.C > new file mode 100644 > index 0000000..65beff1 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/inline-1.C > @@ -0,0 +1,14 @@ > +// { dg-options "-Waddress" } > + > +inline void > +foo (void) > +{ > +} > + > +int > +bar (void) > +{ > + return foo == 0; // { dg-warning "never be NULL" } > +} > + > +// { dg-final { scan-assembler-not "foo" } } > -- > 2.0.0.rc2 >
On Tue, May 27, 2014 at 3:33 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Mon, May 26, 2014 at 9:01 PM, Patrick Palka <patrick@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.
On Tue, May 27, 2014 at 8:32 AM, Patrick Palka <patrick@parcs.ath.cx> wrote: > On Tue, May 27, 2014 at 3:33 AM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Mon, May 26, 2014 at 9:01 PM, Patrick Palka <patrick@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. Oh, disregard the above. I think I see what you mean now, Richard. Because __foo is an alias for foo, and __foo is weak, is it safe to assume that foo is non-NULL? That is a tricky question...
On Tue, May 27, 2014 at 4:06 PM, Patrick Palka <patrick@parcs.ath.cx> wrote: > On Tue, May 27, 2014 at 8:32 AM, Patrick Palka <patrick@parcs.ath.cx> wrote: >> On Tue, May 27, 2014 at 3:33 AM, Richard Biener >> <richard.guenther@gmail.com> wrote: >>> On Mon, May 26, 2014 at 9:01 PM, Patrick Palka <patrick@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. > > Oh, disregard the above. I think I see what you mean now, Richard. > Because __foo is an alias for foo, and __foo is weak, is it safe to > assume that foo is non-NULL? That is a tricky question... No (I wasn't making a folding validity remark). I want to make sure we won't see diagnostics on code like #include <string.h> int main() { if (malloc != 0) ... } just because it is built with -D_FORTIFY_SOURCE. Whether we may fold that check at all is another question, of course (and again that should _not_ differ between -D_FORTIFY_SOURCE or -U_FORTIFY_SOURCE, but it may well also be a glibc issue). And yes, aliases are tricky beasts ... Richard.
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 6ec14fc..d4747a0 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -4508,7 +4508,9 @@ decl_with_nonnull_addr_p (const_tree expr) return (DECL_P (expr) && (TREE_CODE (expr) == PARM_DECL || TREE_CODE (expr) == LABEL_DECL - || !DECL_WEAK (expr))); + || !DECL_WEAK (expr) + || (TREE_CODE (expr) == FUNCTION_DECL + && DECL_DECLARED_INLINE_P (expr)))); } /* Prepare expr to be an argument of a TRUTH_NOT_EXPR, diff --git a/gcc/fold-const.c b/gcc/fold-const.c index 188b179..2796079 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -16052,7 +16052,10 @@ tree_single_nonzero_warnv_p (tree t, bool *strict_overflow_p) || (DECL_CONTEXT (base) && TREE_CODE (DECL_CONTEXT (base)) == FUNCTION_DECL && auto_var_in_fn_p (base, DECL_CONTEXT (base))))) - return !VAR_OR_FUNCTION_DECL_P (base) || !DECL_WEAK (base); + return !VAR_OR_FUNCTION_DECL_P (base) + || !DECL_WEAK (base) + || (TREE_CODE (base) == FUNCTION_DECL + && DECL_DECLARED_INLINE_P (base)); /* Constants are never weak. */ if (CONSTANT_CLASS_P (base)) diff --git a/gcc/testsuite/g++.dg/inline-1.C b/gcc/testsuite/g++.dg/inline-1.C new file mode 100644 index 0000000..65beff1 --- /dev/null +++ b/gcc/testsuite/g++.dg/inline-1.C @@ -0,0 +1,14 @@ +// { dg-options "-Waddress" } + +inline void +foo (void) +{ +} + +int +bar (void) +{ + return foo == 0; // { dg-warning "never be NULL" } +} + +// { dg-final { scan-assembler-not "foo" } }