From patchwork Mon Feb 8 03:45:27 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Wohlferd X-Patchwork-Id: 580154 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id EE0391402D9 for ; Mon, 8 Feb 2016 14:46:17 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=YCo0VcSp; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=cb5lUGmHFfoZQaksR awdRS4LA4LMjXdDrGk0Y8DiQR2AswOCXnmr1wGs10VqgBi20DKzfwODIHofqMBkt uyW45pRIex5HhHGsEkZI4P8VTn2ysgvwrbGzF9KcruZHorzJ+5jpnGIwrR/O6jVT UNyiky34DmVQif+//0pCFDZM4Q= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=tO0USO0JnTb0o7bLOvA5xbE +q2s=; b=YCo0VcSpyElMee0bIsyp95sY0Cpiatw0nyckLvbe/ThnYtTpn+HMRmh 6yXUV1vrxmfhr7q95otHx+Nfzn6INpiamHh0nLX0WrewS/+MFLAWjS9QFvxPMv/N zwksRy5HGE46XSKeNT++QKWKalJgbGrBjQRNtWJHsouCRVoxe9eg= Received: (qmail 126421 invoked by alias); 8 Feb 2016 03:46:10 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 126403 invoked by uid 89); 8 Feb 2016 03:46:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=1.0 required=5.0 tests=AWL, BAYES_50, KAM_ASCII_DIVIDERS, LIKELY_SPAM_BODY, RCVD_IN_DNSWL_NONE, RP_MATCHES_RCVD, SPF_PASS autolearn=no version=3.3.2 spammy=advertising, Lose, guidance, underway X-HELO: bosmailout01.eigbox.net Received: from bosmailout01.eigbox.net (HELO bosmailout01.eigbox.net) (66.96.190.1) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Mon, 08 Feb 2016 03:46:06 +0000 Received: from bosmailscan08.eigbox.net ([10.20.15.8]) by bosmailout01.eigbox.net with esmtp (Exim) id 1aScm3-0006zJ-Cb for gcc-patches@gcc.gnu.org; Sun, 07 Feb 2016 22:46:03 -0500 Received: from [10.115.3.32] (helo=bosimpout12) by bosmailscan08.eigbox.net with esmtp (Exim) id 1aScm3-0007vt-7q for gcc-patches@gcc.gnu.org; Sun, 07 Feb 2016 22:46:03 -0500 Received: from bosauthsmtp18.yourhostingaccount.com ([10.20.18.18]) by bosimpout12 with id Fflx1s0050PPbB601fm0m1; Sun, 07 Feb 2016 22:46:03 -0500 X-Authority-Analysis: v=2.1 cv=BfVW09d2 c=1 sm=1 tr=0 a=hsonH7E3nOZjOFuRJksPKA==:117 a=sZx1nW7oDdbgogxTPqu5Xw==:17 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=88b2x-oFWvEA:10 a=jFJIQSaiL_oA:10 a=r77TgQKjGQsHNAKrUKIA:9 a=mDV3o1hIAAAA:8 a=C8F9KGFtAAAA:8 a=AgeaKr-9NJKguJVWvfAA:9 a=WcZY_QAFIgvVifEb:21 a=Z2XF4PoNCYsDosv6:21 a=QEXdDO2ut3YA:10 a=JKmFtq4-eAMA:10 a=IPECJyk6XdEdxy7DGkMA:9 a=6q7fhYwmMtUvtO3n:21 a=5hxhG3sqmlnpgKnJ:21 Received: from [207.118.20.56] (port=50513 helo=[192.168.1.159]) by bosauthsmtp18.eigbox.net with esmtpa (Exim) id 1aSclv-0005qN-Lr; Sun, 07 Feb 2016 22:45:56 -0500 Subject: Re: Wonly-top-basic-asm To: Bernd Schmidt , "gcc-patches@gcc.gnu.org" , Richard Henderson , jason@redhat.com References: <56A54EF9.8060006@LimeGreenSocks.com> <56A61442.3090803@redhat.com> <56A9C134.1030500@LimeGreenSocks.com> Cc: segher@kernel.crashing.org, sandra@codesourcery.com, Paul_Koning@Dell.com, Jeff Law , bernds_cb1@t-online.de, Bernd Edlinger , Andrew Haley , David Wohlferd From: David Wohlferd Message-ID: <56B80F57.9020606@LimeGreenSocks.com> Date: Sun, 7 Feb 2016 19:45:27 -0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <56A9C134.1030500@LimeGreenSocks.com> X-EN-UserInfo: 97390230d6758ac7ebdf93f8c6197d31:931c98230c6409dcc37fa7e93b490c27 X-EN-AuthUser: dw@limegreensocks.com X-EN-OrigIP: 207.118.20.56 X-EN-OrigHost: unknown Hey Bernd. I replied with a patch that includes most of the changes you asked for (see inline below). Were you waiting on me for something more? I have cleaned up the testcases so they aren't so i386-specific, but otherwise this patch (attached) is the same. Let me know if there is something more I need to do here. Thanks, dw On 1/27/2016 11:20 PM, David Wohlferd wrote: > Rumors that I earn a commission for every person I switch to the > "extended asm" plan are completely unfounded... :) > > That said, I truly believe there are very few cases where using basic > asm within a function makes sense. What's more, either they currently > work incorrectly and need to be found and fixed, or they are going to > take a performance hit when the new "clobber everything" semantics are > implemented. So yeah, I pushed pretty hard to have the docs say > "DON'T USE IT!" > > But reading it all again, you are right: It's too much. > > On 1/25/2016 4:25 AM, Bernd Schmidt wrote: >> On 01/24/2016 11:23 PM, David Wohlferd wrote: >>> +Wonly-top-basic-asm >>> +C ObjC ObjC++ C++ Var(warn_only_top_basic_asm) Warning >>> +Warn on unsafe uses of basic asm. >> >> Maybe just -Wbasic-asm? > > Hmm. Maybe. I've never been completely satisfied with that name. But > wouldn't this sound like it would warn on all uses of basic asm? The > intent is to only flag the ones in functions. They are the ones with > all the problems. > > What would you say to -Wbasic-asm-in-function? A little verbose... > >>> + /* Warn on basic asm used inside of functions, >>> + EXCEPT when in naked functions. Also allow asm(""). */ >> >> Two spaces after a sentence. > > Fixed. > >>> + if (warn_only_top_basic_asm && (TREE_STRING_LENGTH (str) != 1) ) >> >> Unnecessary parens, and extra space before closing paren. > > Fixed. > >>> + if (warn_only_top_basic_asm && >>> + (TREE_STRING_LENGTH (string) != 1)) >> >> Extra parens, and && goes first on the next line. > > Fixed. > >>> + warning_at(asm_loc, OPT_Wonly_top_basic_asm, >> >> Space before "(". > > Fixed. > >>> + "asm statement in function does not use extended >>> syntax"); >> >> Could break that into ".." "..." over two lines so as to keep >> indentation. > > Fixed. > >>> -asm (""); >>> +asm ("":::); >> >> Is that necessary? As far as I can tell we're treating these equally. > > While you are correct that today they are treated (nearly) equally, if > Jeff does what he plans for v7, asm("") is going to translate (on x64) > to: > > asm("":::"rax", "rbx", "rcx", "rdx", "r8", "r9", "r10", "r11", "r12", > "r13", "r14", "r15", "rdi", "rsi", "rbp", "cc", "memory"); > > Given that, it seemed appropriate for the docs to start steering > people away from basic. This particular instance was just something > that was mentioned during the discussion. > > However, that's for someday. Maybe "someday" will turn into some > other solution. And since I'm not prepared to go thru all the docs > and change all the other basic asms at this time, I removed this change. > >>> @@ -7487,6 +7490,8 @@ >>> consecutive in the output, put them in a single multi-instruction >>> @code{asm} >>> statement. Note that GCC's optimizers can move @code{asm} statements >>> relative to other code, including across jumps. >>> +Using inputs and outputs with extended @code{asm} can help >>> correctly position >>> +your asm. >> >> Not sure this is needed either. Sounds a bit like advertising :) In >> general the doc changes seem much too verbose to me. > > I didn't think about this until it was brought up during the > discussion. But once it was pointed out to me, it made sense. > > However, at your suggestion I have removed this. > >>> +Extended @code{asm}'s @samp{%=} may help resolve this. >> >> Same here. I think the block that recommends extended asm is good >> enough. I think the next part could be shrunk significantly too. > > Removed. > >>> -Here is an example of basic @code{asm} for i386: >>> +Basic @code{asm} statements within functions do not perform an >>> implicit >>> +"memory" clobber (@pxref{Clobbers}). Also, there is no implicit >>> clobbering >>> +of @emph{any} registers, so (other than "naked" functions which >>> follow the >> >> "other than in"? Also @code{naked} maybe. > > It works for me either way. Since it bothers you, I changed it. > >> I'd place a note about clobbering after the existing "To access C >> data, it is better to use extended asm". >> >>> +ABI rules) changed registers must be restored to their original >>> value before >>> +exiting the @code{asm}. While this behavior has not always been >>> documented, >>> +GCC has worked this way since at least v2.95.3. Also, lacking >>> inputs and >>> +outputs means that GCC's optimizers may have difficulties consistently >>> +positioning the basic @code{asm} in the generated code. >> >> The existing text already mentions ordering issues. Lose this block. > > I've removed the rest of the paragraph after "Also" > > Wait, didn't you tell me to remove the other mention of 'ordering'? I > think I've removed all of them now. Not a huge loss, but was that > what you intended? > >>> +The concept of ``clobbering'' does not apply to basic @code{asm} >>> statements >>> +outside of functions (aka top-level asm). >> >> Stating the obvious? > > I don't actually agree with this one. However I do agree with your > comment re being too verbose. Since the new sample shows how to do > this (using extended asm for clobbers), I have removed this. > >>> +@strong{Warning!} This "clobber nothing" behavior may be different >>> than how >> >> Ok there is precedent for this, but it's spelt "@strong{Warning:}" in >> all other cases. > > Fixed. > >> Still, I'd probably also shrink this paragraph and put a note about >> lack of C semantics and possibly different behaviour from other >> compilers near the top, where we say that extended asm is better in >> most cases. >> >>> +other compilers treat basic @code{asm}, since the C standards for the >>> +@code{asm} statement provide no guidance regarding these >>> semantics. As a >>> +result, @code{asm} statements that work correctly on other >>> compilers may not >>> +work correctly with GCC (and vice versa), even though they both >>> compile >>> +without error. Also, there is discussion underway about changing >>> GCC to >>> +have basic @code{asm} clobber at least memory and perhaps some (or >>> all) >>> +registers. If implemented, this change may fix subtle problems with >>> +existing @code{asm} statements. However it may break or slow down >>> ones that >>> +were working correctly. >> >> How would such a change break anything? I'd also not mention >> discussion underway, just say "Future versions of GCC may treat basic >> @code{asm} as clobbering memory". > > Removed 'discussion underway', but kept references to clobbering > registers. That's Jeff's expressed intent, and part of why I believe > people will want to find these statements. > >>> +If your existing code needs clobbers that GCC's basic @code{asm} is >>> not >>> +providing, or if you want to 'future-proof' your asm against possible >>> +changes to basic @code{asm}'s semantics, use extended @code{asm}. >> >> Recommending it too often. Lose this. >> >>> +Extended @code{asm} allows you to specify what (if anything) needs >>> to be >>> +clobbered for your code to work correctly. >> >> And again. > > I believe I have removed and replaced all the recurring redundant > repetitiveness. > >>> You can use @ref{Warning >>> +Options, @option{-Wonly-top-basic-asm}} to locate basic @code{asm} >> >> I think just plain @option is usual. > > You are recommending I remove the link to the warning options? While > I'm normally a big fan of links, I defer to your judgement. Removed. > >>> +statements that may need changes, and refer to >>> +@uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to >>> convert >>> +from basic asm to extended asm} for information about how to >>> perform the >>> +conversion. >> >> A link is probably good if we have such a page. > > We do have such a page, but it was written by the same "too verbose" > guy that wrote this patch. Might be worth a review if you have a minute. > >>> +Here is an example of top-level basic @code{asm} for i386 that >>> defines an >>> +asm macro. That macro is then invoked from within a function using >>> +extended @code{asm}: >> >> The updated example also looks good. > > There are legitimate uses for basic asm. While this sample is still > trivial, it shows a much more important concept than the old one. > >> I think I'm fine with the concept > > Thanks for the detailed review. > >> but I'd like to see an updated patch with better docs. > > Updated patch attached. Now includes testsuites. The updated web > page is at http://www.limegreensocks.com/gcc/Basic-Asm.html > > dw Index: gcc/c-family/c.opt =================================================================== --- gcc/c-family/c.opt (revision 233206) +++ gcc/c-family/c.opt (working copy) @@ -585,6 +585,10 @@ C++ ObjC++ Var(warn_namespaces) Warning Warn on namespace definition. +Wonly-top-basic-asm +C ObjC ObjC++ C++ Var(warn_only_top_basic_asm) Warning +Warn on unsafe uses of basic asm. + Wsized-deallocation C++ ObjC++ Var(warn_sized_deallocation) Warning EnabledBy(Wextra) Warn about missing sized deallocation functions. Index: gcc/c/c-parser.c =================================================================== --- gcc/c/c-parser.c (revision 233206) +++ gcc/c/c-parser.c (working copy) @@ -5972,7 +5972,18 @@ labels = NULL_TREE; if (c_parser_next_token_is (parser, CPP_CLOSE_PAREN) && !is_goto) + { + /* Warn on basic asm used inside of functions, + EXCEPT when in naked functions. Also allow asm(""). */ + if (warn_only_top_basic_asm && TREE_STRING_LENGTH (str) != 1) + if (lookup_attribute ("naked", + DECL_ATTRIBUTES (current_function_decl)) + == NULL_TREE) + warning_at (asm_loc, OPT_Wonly_top_basic_asm, + "asm statement in function does not use extended syntax"); + goto done_asm; + } /* Parse each colon-delimited section of operands. */ nsections = 3 + is_goto; Index: gcc/cp/parser.c =================================================================== --- gcc/cp/parser.c (revision 233206) +++ gcc/cp/parser.c (working copy) @@ -18021,6 +18021,8 @@ bool goto_p = false; required_token missing = RT_NONE; + location_t asm_loc = cp_lexer_peek_token (parser->lexer)->location; + /* Look for the `asm' keyword. */ cp_parser_require_keyword (parser, RID_ASM, RT_ASM); @@ -18179,6 +18181,17 @@ /* If the extended syntax was not used, mark the ASM_EXPR. */ if (!extended_p) { + /* Warn on basic asm used inside of functions, + EXCEPT when in naked functions. Also allow asm(""). */ + if (warn_only_top_basic_asm + && TREE_STRING_LENGTH (string) != 1) + if (lookup_attribute("naked", + DECL_ATTRIBUTES (current_function_decl)) + == NULL_TREE) + warning_at (asm_loc, OPT_Wonly_top_basic_asm, + "asm statement in function does not use extended" + " syntax"); + tree temp = asm_stmt; if (TREE_CODE (temp) == CLEANUP_POINT_EXPR) temp = TREE_OPERAND (temp, 0); Index: gcc/doc/extend.texi =================================================================== --- gcc/doc/extend.texi (revision 233206) +++ gcc/doc/extend.texi (working copy) @@ -7458,7 +7458,8 @@ @end table @subsubheading Remarks -Using extended @code{asm} typically produces smaller, safer, and more +Using extended @code{asm} (@pxref{Extended Asm}) typically produces smaller, +safer, and more efficient code, and in most cases it is a better solution than basic @code{asm}. However, there are two situations where only basic @code{asm} can be used: @@ -7516,11 +7517,51 @@ Basic @code{asm} provides no mechanism to provide different assembler strings for different dialects. -Here is an example of basic @code{asm} for i386: +Basic @code{asm} statements do not perform an implicit "memory" clobber +(@pxref{Clobbers}). Also, there is no implicit clobbering of @emph{any} +registers, so (other than in @code{naked} functions which follow the ABI +rules) changed registers must be restored to their original value before +exiting the @code{asm}. While this behavior has not always been +documented, GCC has worked this way since at least v2.95.3. +@strong{Warning:} This "clobber nothing" behavior may be different than how +other compilers treat basic @code{asm}, since the C standards for the +@code{asm} statement provide no guidance regarding these semantics. As a +result, @code{asm} statements that work correctly on other compilers may not +work correctly with GCC (and vice versa), even though they both compile +without error. + +Future versions of GCC may change basic @code{asm} to clobber memory and +perhaps some (or all) registers. This change may fix subtle problems with +existing @code{asm} statements. However it may break or slow down ones +that were working correctly. To ``future-proof'' your asm against possible +changes to basic @code{asm}'s semantics, use extended @code{asm}. + +You can use @option{-Wonly-top-basic-asm} to locate basic @code{asm} +statements that may need changes, and refer to +@uref{https://gcc.gnu.org/wiki/ConvertBasicAsmToExtended, How to convert +from basic asm to extended asm} for information about how to perform the +conversion. + +Here is an example of top-level basic @code{asm} for i386 that defines an +asm macro. That macro is then invoked from within a function using +extended @code{asm}: + @example -/* Note that this code will not compile with -masm=intel */ -#define DebugBreak() asm("int $3") +/* Define macro at file scope with basic asm. */ +/* Add macro parameter p to eax. */ +asm(".macro test p\n\t" + "addl $\\p, %eax\n\t" + ".endm"); + +/* Use macro in function using extended asm. It needs */ +/* the "cc" clobber since the flags are changed and uses */ +/* the "a" constraint since it modifies eax. */ +int DoAdd(int value) +@{ + asm("test 5" : "+a" (value) : : "cc"); + return value; +@} @end example @node Extended Asm @@ -8047,7 +8088,7 @@ for @code{d} by specifying both constraints. @anchor{FlagOutputOperands} -@subsection Flag Output Operands +@subsubsection Flag Output Operands @cindex @code{asm} flag output operands Some targets have a special register that holds the ``flags'' for the Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 233206) +++ gcc/doc/invoke.texi (working copy) @@ -5728,6 +5728,21 @@ a structure that has been marked with the @code{designated_init} attribute. +@item -Wonly-top-basic-asm @r{(C and C++ only)} +Warn if basic @code{asm} statements are used inside a function (i.e. not at +top-level/file scope). + +When used inside of functions, basic @code{asm} can result in unexpected and +unwanted variations in behavior between compilers due to how registers are +handled when calling the asm (@pxref{Basic Asm}). The lack of input and +output constraints (@pxref{Extended Asm}) can also make it difficult for +optimizers to correctly and consistently position the output relative to +other code. + +Functions that are marked with the @option{naked} attribute (@pxref{Function +Attributes}) and @code{asm} statements with an empty instruction string are +excluded from this check. + @item -Whsa Issue a warning when HSAIL cannot be emitted for the compiled function or OpenMP construct. Index: gcc/testsuite/c-c++-common/Wonly-top-basic-asm-2.c =================================================================== --- gcc/testsuite/c-c++-common/Wonly-top-basic-asm-2.c (revision 0) +++ gcc/testsuite/c-c++-common/Wonly-top-basic-asm-2.c (working copy) @@ -0,0 +1,15 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target naked_functions } */ +/* { dg-options "-Wonly-top-basic-asm" } */ + +int __attribute__((naked)) +func (int x, int y) +{ + /* basic asm should not warn in naked functions. */ + asm(" "); /* no warning */ +} + +int main(int argc, char *argv[]) +{ + return func(argc, argc); +} Index: gcc/testsuite/c-c++-common/Wonly-top-basic-asm.c =================================================================== --- gcc/testsuite/c-c++-common/Wonly-top-basic-asm.c (revision 0) +++ gcc/testsuite/c-c++-common/Wonly-top-basic-asm.c (working copy) @@ -0,0 +1,43 @@ +/* { dg-do compile } */ +/* { dg-options "-Wonly-top-basic-asm" } */ + +#if defined(__i386__) || defined(__x86_64__) +/* acceptable */ +register int b asm("esi"); +#else +int b = 3; +#endif + +/* acceptable */ +int foo asm ("myfoo") = 2; + +/* acceptable */ +asm (" "); + +/* acceptable */ +int func (int x, int y) asm ("MYFUNC"); + +int main(int argc, char *argv[]) +{ +#if defined(__i386__) || defined(__x86_64__) + /* acceptable */ + register int a asm("edi"); +#else + int a = 2; +#endif + + /* acceptable */ + asm(" "::"r"(a), "r" (b)); + + /* acceptable */ + asm goto (""::::done); + + /* acceptable */ + asm(""); + + /* warning */ + asm(" "); /* { dg-warning "does not use extended syntax" } */ + + done: + return 0; +}