From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 51409 invoked by alias); 11 Jan 2019 22:05:18 -0000 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 Received: (qmail 51399 invoked by uid 89); 11 Jan 2019 22:05:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS autolearn=no version=3.3.2 spammy=look, pm, so, make X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 11 Jan 2019 22:05:15 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 17F6137E60; Fri, 11 Jan 2019 22:05:14 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-2.rdu2.redhat.com [10.10.112.2]) by smtp.corp.redhat.com (Postfix) with ESMTP id D22685D757; Fri, 11 Jan 2019 22:05:12 +0000 (UTC) Subject: Re: [PATCH] restore CFString handling in attribute format (PR 88638) To: Martin Sebor , Iain Sandoe Cc: "gcc-patches@gcc.gnu.org" , Mike Stump References: <7F4803D9-451F-4DFC-AC93-CDA68F742693@comcast.net> <4E249F38-0BA6-4937-BFBC-22D4D93331A3@googlemail.com> <338aa29f-9700-26d5-909b-c7cdc2f3cdd4@gmail.com> <576B95E8-3086-426B-BFDC-EB98793713F3@googlemail.com> <534c6815-7b42-522d-1974-23c5b1593fa2@gmail.com> From: Jeff Law Openpgp: preference=signencrypt Message-ID: <2146d0d5-d92b-3e22-68ae-7e16171e3c68@redhat.com> Date: Fri, 11 Jan 2019 22:05:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1 MIME-Version: 1.0 In-Reply-To: <534c6815-7b42-522d-1974-23c5b1593fa2@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2019-01/txt/msg00671.txt.bz2 On 1/6/19 4:34 PM, Martin Sebor wrote: > Attached is an updated patch with the wording change to the manual > discussed below and rebased on the top of today's trunk. > > Martin > > PS Thanks for the additional info, Iain. > > On 1/5/19 10:53 AM, Iain Sandoe wrote: >> >>> On 5 Jan 2019, at 17:39, Martin Sebor  wrote: >>> >>> On 1/5/19 3:31 AM, Iain Sandoe wrote: >>>> Hi Martin, >>>>> On 4 Jan 2019, at 22:30, Mike Stump  wrote: >>>>> >>>>> On Jan 4, 2019, at 2:03 PM, Martin Sebor  wrote: >>>>>> >>>>>> The improved handling of attribute positional arguments added >>>>>> in r266195 introduced a regression on Darwin where attribute >>>>>> format with the CFString archetype accepts CFString* parameter >>>>>> types in positions where only char* would otherwise be allowed. >>>>> >>>>> You didn't ask Ok?  I'll assume you want a review...  The darwin bits and the testsuite bits look fine. >>>>> >>>>>> >>>>>> Index: gcc/doc/extend.texi >>>>>> =================================================================== >>>>>> --- gcc/doc/extend.texi    (revision 267580) >>>>>> +++ gcc/doc/extend.texi    (working copy) >>>>>> @@ -22368,10 +22368,12 @@ bit-fields.  See the Solaris man page for @code{cm >>>>>> >>>>>>   @node Darwin Format Checks >>>>>>   @subsection Darwin Format Checks >>>>>>   -Darwin targets support the @code{CFString} (or @code{__CFString__}) in the format >>>>>> >>>>>> -attribute context.  Declarations made with such attribution are parsed for correct syntax >>>>>> >>>>>> -and format argument types.  However, parsing of the format string itself is currently undefined >>>>>> >>>>>> -and is not carried out by this version of the compiler. >>>>>> +In addition to the full set of archetypes, Darwin targets also support >>>>>> >>>>>> +the @code{CFString} (or @code{__CFString__}) archetype in the @code{format} >>>>>> >>>>>> +attribute.  Declarations with this archetype are parsed for correct syntax >>>>>> >>>>>> +and argument types.  However, parsing of the format string itself and >>>>>> >>>>>> +validating arguments against it in calls to such functions is currently >>>>>> >>>>>> +not performed. >>>>>>     Additionally, @code{CFStringRefs} (defined by the @code{CoreFoundation} headers) may >>>>>> >>>>>>   also be used as format arguments.  Note that the relevant headers are only likely to be >>>>>> >>>>>> >>>> I find “archetype(s)” to be an usual (and possibly unfamiliar to many) word for this context. >>>> >>>> how about: >>>> s/archetype in/variant for the/ >>>> and then >>>>   s/with this archetype/with this variant/ >>>> in the following sentence. >>>> However, just 0.02GBP .. (fixing the fails is more important than bikeshedding the wording). >>>> >>> >>> Thanks for chiming in!  I used archetype because that's the term >>> used in the attribute format specification to describe the first >>> argument.  I do tend to agree that archetype alone may not be >>> sufficiently familiar to all users.  I'm happy to add text to >>> make that clear.  Would you find the following better? >>> >>>   In addition to the full set of format archetypes (attribute >>>   format style arguments such as @code{printf}, @code{scanf}, >>>   @code{strftime}, and @code{strfmon}), Darwin targets also >>>   support the @code{CFString} (or @code{__CFString__}) archetype… >> >> Yes, that makes is clearer >> >> (as an aside, I think that to many people the meaning of archetype - as ‘generic’  or ‘root example’ >> >>    etc  tends to come to mind before the ‘template/mold’ meaning … however couldn’t think of >> >>   a better term that’s not already overloaded). >> >>> FWIW, I wanted to figure out how the CFString attribute made it >>> possible to differentiate between printf and scanf (and the other) >>> kinds of functions, for example, so I could add new tests for it, >>> but I couldn't tell that from the manual.  So I'm trying to update >>> the text to make it clear that although CFString is just like >>> the sprintf and scanf format arguments/archetypes, beyond >>> validating declarations that use it, the attribute serves no >>> functional purpose, so the printf/scanf distinction is moot. >> >> The CFString container** is more general than our implementation, e.g. it should be able >> >> to contain a variety of string formats (e.g. UTF etc.).   AFAIR at the time I implemented >> >> it for FSF GCC (it was originally in the Apple Local branch), we didn’t have sufficient parsing >> >> support for such things (and the support in the Apple-local branch didn’t look applicable). >> >> >> If we do more sophisitcated checks, we probably need to take cognisance of the fact that a >> >> fully-implemented CFString impl can have non-ascii payload.   I suspect (although honestly >> >> it’s been a while, I maybe mis-remember) that was the reason I didn’t try to implement the >> >> inspection at the time (if so, there’s probably a code comment to that effect). >> >> >>> Out of curiosity, is the attribute used for function call >>> validation by compilers other than GCC?  I couldn't find >>> anything online. >> >> It used to be used for the platform GCC, when that was the “system compiler" (last edition >> >>   apple 4.2.1) and I therefore assume it’s used by clang - but actually haven’t double-checked >> >> at the time we added it - it was relevant. >> >> Let’s revisit this in 10, and see if we can’t sync up with the platform expectations from the clang impl. >> >> >> thanks for taking care of this, >> >> Iain >> >> ** it’s actually a simple ObjectiveC object that happens to be supported by the CoreFoundation (C) >> >> classes as well as ObjectiveC .. making it possible to pass around general string containers between >> >> the languages. >> > > > gcc-88638.diff > > PR target/88638 - FAIL: fsf-nsstring-format-1.s on darwin > > gcc/c-family/ChangeLog: > > PR target/88638 > * c-attribs.c (positional_argument): Call valid_format_string_type_p > and issue errors if it fails. > * c-common.h (valid_format_string_type_p): Declare. > * c-format.c (valid_stringptr_type_p): Rename... > (valid_format_string_type_p): ...to this and make extern. > (handle_format_arg_attribute): Adjust to new name. > (check_format_string): Same. > > gcc/testsuite/ChangeLog: > > PR target/88638 > * gcc.dg/format/attr-8.c: New test. > * gcc.dg/darwin-cfstring-format-1.c: Adjust diagnostics. > * gcc.dg/format/attr-3.c: Same. > * obj-c++.dg/fsf-nsstring-format-1.mm: Same. > * objc.dg/fsf-nsstring-format-1.m: Same. > > gcc/ChangeLog: > > PR target/88638 > * doc/extend.texi (Darwin Format Checks): Clarify. OK jeff