From patchwork Thu Jan 28 07:20:20 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Wohlferd X-Patchwork-Id: 574562 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 A9D7B14012C for ; Thu, 28 Jan 2016 18:21:15 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=v2N3UkWr; 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=uX6YAdq132a8ot8Zp yJK/oCOtw7QZehcWkPNjMXx+qGCun5qbtmfHKQknxtVPG624czPKuVszO2F1v07O Vf3pknKsJHZrshdA6u1tgjBoR1boCOepH/QGZIN/O+7aPb2fdHy7z8b39MRkjc6C FTC4RyompBw6/Wusxpf2s7blrQ= 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=w/bgaO0DHofkPmdS39IfV11 ayCg=; b=v2N3UkWrblTiICLIqXwzRnh8hKhXBvsKBU6tm01FFoua+iUb40/43b+ vgEsyZbh8jsVLTSDQBphK5Aox5luiq8cIXx7tATYD641n+a1G2dcw484oLe1AYi8 wFwD/jfItGM8rDw7nz7p23DqJtEO8r1uvEfnHrm6Tu78m6x4y+94= Received: (qmail 125708 invoked by alias); 28 Jan 2016 07:21:07 -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 125691 invoked by uid 89); 28 Jan 2016 07:21:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.6 required=5.0 tests=AWL, BAYES_00, KAM_ASCII_DIVIDERS, LIKELY_SPAM_BODY, RCVD_IN_DNSWL_LOW, RP_MATCHES_RCVD, SPF_PASS autolearn=no version=3.3.2 spammy=advertising, IT, IT!, x64 X-HELO: bosmailout10.eigbox.net Received: from bosmailout10.eigbox.net (HELO bosmailout10.eigbox.net) (66.96.185.10) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Thu, 28 Jan 2016 07:21:04 +0000 Received: from bosmailscan04.eigbox.net ([10.20.15.4]) by bosmailout10.eigbox.net with esmtp (Exim) id 1aOgt3-00064M-T6 for gcc-patches@gcc.gnu.org; Thu, 28 Jan 2016 02:21:01 -0500 Received: from [10.115.3.32] (helo=bosimpout12) by bosmailscan04.eigbox.net with esmtp (Exim) id 1aOgt2-00058q-Sq for gcc-patches@gcc.gnu.org; Thu, 28 Jan 2016 02:21:00 -0500 Received: from bosauthsmtp18.yourhostingaccount.com ([10.20.18.18]) by bosimpout12 with id BKLq1s00B0PPbB601KLttA; Thu, 28 Jan 2016 02:21:00 -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=7aQ_Q-yQQ-AA:10 a=r77TgQKjGQsHNAKrUKIA:9 a=mDV3o1hIAAAA:8 a=C8F9KGFtAAAA:8 a=KBJJzaGc2xxLvkbGuMoA:9 a=wQaRK75N6TorT66Q:21 a=Cl957S3IjmqBDFQ3:21 a=QEXdDO2ut3YA:10 a=JKmFtq4-eAMA:10 a=IPECJyk6XdEdxy7DGkMA:9 a=mB39x7BZkKUcZm5b:21 a=QzETlt9onDipwOlQ:21 Received: from [207.118.20.56] (port=50891 helo=[192.168.1.159]) by bosauthsmtp18.eigbox.net with esmtpa (Exim) id 1aOgsr-00063n-QN; Thu, 28 Jan 2016 02:20:50 -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> Cc: segher@kernel.crashing.org, sandra@codesourcery.com, Paul_Koning@Dell.com, Jeff Law , bernds_cb1@t-online.de, Bernd Edlinger , Andrew Haley From: David Wohlferd Message-ID: <56A9C134.1030500@LimeGreenSocks.com> Date: Wed, 27 Jan 2016 23:20:20 -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: <56A61442.3090803@redhat.com> X-EN-UserInfo: 97390230d6758ac7ebdf93f8c6197d31:931c98230c6409dcc37fa7e93b490c27 X-EN-AuthUser: dw@limegreensocks.com X-EN-OrigIP: 207.118.20.56 X-EN-OrigHost: unknown 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 232773) +++ 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 232773) +++ gcc/c/c-parser.c (working copy) @@ -5973,7 +5973,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 232773) +++ gcc/cp/parser.c (working copy) @@ -18003,6 +18003,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); @@ -18161,6 +18163,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 232773) +++ 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 232773) +++ gcc/doc/invoke.texi (working copy) @@ -5693,6 +5693,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,35 @@ +/* { dg-do compile } */ +/* { dg-options "-Wonly-top-basic-asm" } */ + +/* acceptable */ +register int b asm("esi"); + +/* acceptable */ +int foo asm ("myfoo") = 2; + +/* acceptable */ +asm ("# top level"); + +/* acceptable */ +int func (int x, int y) asm ("MYFUNC"); + +int main(int argc, char *argv[]) +{ + /* acceptable */ + register int a asm("edi"); + + /* acceptable */ + asm("#"::"r"(a), "r" (b)); + + /* acceptable */ + asm goto (""::::done); + + /* acceptable */ + asm(""); + + /* warning */ + asm(" "); /* { dg-warning "does not use extended syntax" } */ + + done: + return 0; +}