From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30218 invoked by alias); 31 May 2011 16:04:31 -0000 Received: (qmail 30209 invoked by uid 22791); 31 May 2011 16:04:30 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from eu1sys200aog107.obsmtp.com (HELO eu1sys200aog107.obsmtp.com) (207.126.144.123) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 31 May 2011 16:03:58 +0000 Received: from beta.dmz-eu.st.com ([164.129.1.35]) (using TLSv1) by eu1sys200aob107.postini.com ([207.126.147.11]) with SMTP ID DSNKTeURbJyB9fCiwMOJjnIrQLir+LeEurqg@postini.com; Tue, 31 May 2011 16:03:58 UTC Received: from zeta.dmz-eu.st.com (ns2.st.com [164.129.230.9]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 5937A112; Tue, 31 May 2011 16:03:40 +0000 (GMT) Received: from Webmail-eu.st.com (safex1hubcas2.st.com [10.75.90.16]) by zeta.dmz-eu.st.com (STMicroelectronics) with ESMTP id B71A429AE; Tue, 31 May 2011 16:03:40 +0000 (GMT) Received: from [164.129.122.89] (164.129.122.89) by webmail-eu.st.com (10.75.90.13) with Microsoft SMTP Server (TLS) id 8.2.234.1; Tue, 31 May 2011 18:03:40 +0200 Message-ID: <4DE5115B.4070409@st.com> Date: Tue, 31 May 2011 17:07:00 -0000 From: Christian Bruel User-Agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.17) Gecko/20110414 Thunderbird/3.1.10 ThunderBrowse/3.3.5 MIME-Version: 1.0 To: Richard Guenther Cc: GCC Patches Subject: Re: [PATH] PR/49139 fix always_inline failures diagnostics References: <4DE49EBE.30804@st.com> In-Reply-To: Content-Type: multipart/mixed; boundary="------------010005070408060105070701" X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2011-05/txt/msg02442.txt.bz2 --------------010005070408060105070701 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Content-length: 3797 On 05/31/2011 11:18 AM, Richard Guenther wrote: > On Tue, May 31, 2011 at 9:54 AM, Christian Bruel wrote: >> Hello, >> >> The attached patch fixes a few diagnostic discrepancies for always_inline >> failures. >> >> Illustrated by the fail_always_inline[12].c attached cases, the current >> behavior is one of: >> >> - success (with and without -Winline), silently not honoring always_inline >> gcc fail_always_inline1.c -S -Winline -O0 -fpic >> gcc fail_always_inline1.c -S -O2 -fpic >> >> - error: with -Winline but not without >> gcc fail_always_inline1.c -S -Winline -O2 -fpic >> >> - error: without -Winline >> gcc fail_always_inline2.c -S -fno-early-inlining -O2 >> or the original c++ attachment in this defect >> >> note that -Winline never warns, as stated in the documentation >> >> This simple patch consistently emits a warning (changing the sorry >> unimplemented message) whenever the attribute is not honored. >> >> My first will was to generate and error instead of the warning, but since it >> is possible that inlines is only performed at LTO time, an error would be >> inapropriate (Note that today this is not possible with -Winline that would >> abort). >> >> Another alternative I considered would be to emit the warning under -Winline >> rather than unconditionally, but this more a user misuse of the attribute, >> so should always be warned anyway. Or maybe a new -Winline-always that would >> be activated under -Wall ? Other opinion welcomed. >> >> Tested with standard bootstrap and regression on x86. >> >> Comments, and/or OK for trunk ? > > The patch is not ok, we may not fail to inline an always_inline > function. OK, I thought so that would be an error. but I was afraid to abort the inline of function with a missing body (provided by another file) by LTO, which would be a regression. rethinking about this and considering that a valid LTO program should be valid without LTO, and that the scope is the translation unit, that would be OK to always reject attribute_inline on functions without a body. To make this more consistent I proposed to warn > whenever you take the address of an always_inline function > (because then you can confuse GCC by indirectly calling > such function which we might inline dependent on optimization > setting and which we might discover we didn't inline only > dependent on optimization setting).Honza proposed to move > the sorry()ing to when we feel the need to output the > always_inline function, thus when it was not optimized away, > but that would require us not preserving the body (do we?) > with -fpreserve-inline-functions. > But we don't know if taking the address of the function will result by a call to it, or how the pointer will be used. So I think the check should be done at the caller site. But I code like: inline __attribute__((always_inline)) int foo() { return 0; } static int (*ptr)() = foo; int bar() { return ptr(); } is not be (legitimately) inlined, but without diagnostic, I don't know where to look at this that at the moment. > For fail_always_inline1.c we should diagnose the appearant > misuse of always_inline with a warning, drop the attribute > but keep DECL_DISREGARD_INLINE_LIMITS set. > > Same for fail_always_inline2.c. > > I agree that sorry()ing for those cases is odd. EIther we > should reject the declarations upfront > ("always-inline function will not be inlinable"), or we should > emit a warning of that kind and make sure to not sorry later. In addition to the error at the caller site, I've added the specific warn about the missing inline keyword. Thanks for your comments, here is the new patch that I'm testing, (without the handling of indirect calls which can be treated separately). Cheers Christian --------------010005070408060105070701 Content-Type: text/plain; name="always_inline_warn.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="always_inline_warn.patch" Content-length: 4832 2010-05-25 Christian Bruel PR 49139 * cgraph.c (cgraph_function_body_availability): Check always_inline * ipa-inline-transform.c (inline_transform): Force optimize_inline_calls error checking if always_inline seen. * tree-inline.c (tree_inlinable_function_p): Use error instead of sorry. (expand_call_inline): Likewise. 2010-05-25 Christian Bruel * gcc.db/always_inline.c: Removed -Winline. Update checks * gcc.db/always_inline2.c: Likewise. * gcc.db/always_inline3.c: Likewise. * gcc.db/fail_always_inline.c: New. Index: gcc/cgraph.c =================================================================== --- gcc/cgraph.c (revision 174264) +++ gcc/cgraph.c (working copy) @@ -2414,7 +2414,14 @@ bit. */ else if (decl_replaceable_p (node->decl) && !DECL_EXTERNAL (node->decl)) + { + if (cgraph_global_info_ready) + if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (node->decl))) + if (!DECL_DECLARED_INLINE_P (node->decl)) + warning (0, "always_inline function might not be inlinable"); + avail = AVAIL_OVERWRITABLE; + } else avail = AVAIL_AVAILABLE; return avail; Index: gcc/ipa-inline-transform.c =================================================================== --- gcc/ipa-inline-transform.c (revision 174264) +++ gcc/ipa-inline-transform.c (working copy) @@ -302,9 +302,20 @@ for (e = node->callees; e; e = e->next_callee) { - cgraph_redirect_edge_call_stmt_to_callee (e); + gimple call = cgraph_redirect_edge_call_stmt_to_callee (e); + + if (!inline_p) + { if (!e->inline_failed || warn_inline) inline_p = true; + else + { + tree fn = gimple_call_fndecl (call); + + if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (fn))) + inline_p = true; + } + } } if (inline_p) Index: gcc/testsuite/gcc.dg/always_inline.c =================================================================== --- gcc/testsuite/gcc.dg/always_inline.c (revision 174264) +++ gcc/testsuite/gcc.dg/always_inline.c (working copy) @@ -1,8 +1,8 @@ /* { dg-do compile } */ -/* { dg-options "-Winline -O2" } */ +/* { dg-options "-O2" } */ #include inline __attribute__ ((always_inline)) void -e(int t, ...) /* { dg-message "sorry\[^\n\]*variable argument" "" } */ +e(int t, ...) /* { dg-error "variable argument" } */ { va_list q; va_start (q, t); Index: gcc/testsuite/gcc.dg/always_inline2.c =================================================================== --- gcc/testsuite/gcc.dg/always_inline2.c (revision 174264) +++ gcc/testsuite/gcc.dg/always_inline2.c (working copy) @@ -1,8 +1,8 @@ /* { dg-do compile } */ -/* { dg-options "-Winline -O2" } */ -inline __attribute__ ((always_inline)) void t(void); /* { dg-message "sorry\[^\n\]*body not available" "" } */ +/* { dg-options "-O2" } */ +inline __attribute__ ((always_inline)) void t(void); /* { dg-error "body not available" } */ void q(void) { - t(); /* { dg-message "sorry\[^\n\]*called from here" "" } */ + t(); /* { dg-error "called from here" } */ } Index: gcc/testsuite/gcc.dg/always_inline3.c =================================================================== --- gcc/testsuite/gcc.dg/always_inline3.c (revision 174264) +++ gcc/testsuite/gcc.dg/always_inline3.c (working copy) @@ -1,11 +1,11 @@ /* { dg-do compile } */ -/* { dg-options "-Winline -O2" } */ +/* { dg-options "-O2" } */ int do_something_evil (void); inline __attribute__ ((always_inline)) void -q2(void) /* { dg-message "sorry\[^\n\]*recursive" "" } */ +q2(void) /* { dg-error "recursive inlining" } */ { if (do_something_evil ()) return; - q2(); /* { dg-message "sorry\[^\n\]*called from here" "" } */ + q2(); /* { dg-error "called from here" } */ q2(); /* With -O2 we don't warn here, it is eliminated by tail recursion. */ } Index: gcc/tree-inline.c =================================================================== --- gcc/tree-inline.c (revision 174264) +++ gcc/tree-inline.c (working copy) @@ -3192,7 +3192,7 @@ As a bonus we can now give more details about the reason why a function is not inlinable. */ if (always_inline) - sorry (inline_forbidden_reason, fn); + error (inline_forbidden_reason, fn); else if (do_warning) warning (OPT_Winline, inline_forbidden_reason, fn); @@ -3744,9 +3744,9 @@ /* Avoid warnings during early inline pass. */ && cgraph_global_info_ready) { - sorry ("inlining failed in call to %q+F: %s", fn, - _(cgraph_inline_failed_string (reason))); - sorry ("called from here"); + error ("inlining failed in call to always_inline %q+F: %s", fn, + cgraph_inline_failed_string (reason)); + error ("called from here"); } else if (warn_inline && DECL_DECLARED_INLINE_P (fn) --------------010005070408060105070701 Content-Type: text/plain; name="fail_always_inline.c" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="fail_always_inline.c" Content-length: 232 /* { dg-do compile { target fpic } } */ /* { dg-options "-fpic" } */ __attribute((always_inline)) void bar() { } /* { dg-error "can be overwriten at linktime" } */ void f() { bar(); /* { dg-error "called from here" "" } */ } --------------010005070408060105070701--