Hi Jason, On 6 Feb 2025, at 16:48, Jason Merrill wrote:
> On 2/5/25 2:21 PM, Simon Martin wrote: >> Hi Jason, >> >> On 4 Feb 2025, at 21:23, Jason Merrill wrote: >> >>> On 2/4/25 3:03 PM, Jason Merrill wrote: >>>> On 2/4/25 11:45 AM, Simon Martin wrote: >>>>> On 4 Feb 2025, at 17:17, Jason Merrill wrote: >>>>> >>>>>> On 2/4/25 10:56 AM, Simon Martin wrote: >>>>>>> Hi Jason, >>>>>>> >>>>>>> On 4 Feb 2025, at 16:39, Jason Merrill wrote: >>>>>>> >>>>>>>> On 1/15/25 9:56 AM, Jason Merrill wrote: >>>>>>>>> On 1/15/25 7:24 AM, Simon Martin wrote: >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> On 14 Jan 2025, at 23:31, Jason Merrill wrote: >>>>>>>>>> >>>>>>>>>>> On 1/14/25 2:13 PM, Simon Martin wrote: >>>>>>>>>>>> On 10 Jan 2025, at 19:10, Andrew Pinski wrote: >>>>>>>>>>>>> On Fri, Jan 10, 2025 at 3:18 AM Simon Martin >>>>>>>>>>>>> <si...@nasilyan.com> >>>>>>>>>>>>> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> We currently accept the following invalid code (EDG and >>>>>>>>>>>>>> MSVC >>>>>>>>>>>>>> do >>>>>>>>>>>>>> as >>>>>>>>>>>>>> well) >>>>>>>>>>>>> >>>>>>>>>>>>> clang does too: >>>>>>>>>>>>> https://github.com/llvm/llvm-project/issues/121706 . >>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Note it might be useful if a testcase with multiply `*` is >>>>>>>>>>>>> included >>>>>>>>>>>> >>>>>>>>>>>>> too: >>>>>>>>>>>>> ``` >>>>>>>>>>>>> struct A { >>>>>>>>>>>>> ****A (); >>>>>>>>>>>>> }; >>>>>>>>>>>>> ``` >>>>>>>>>>>> Thanks, makes sense to add those. Done in the attached >>>>>>>>>>>> updated >>>>>>>>>>>> revision, >>>>>>>>>>>> successfully tested on x86_64-pc-linux-gnu. >>>>>>>>>>> >>>>>>>>>>>> +/* Check that it's OK to declare a function at ID_LOC with >>>>>>>>>>>> the >>>>>>>>>>>> indicated TYPE, >>>>>>>>>>>> + TYPE_QUALS and DECLARATOR. SFK indicates the kind >>>>>>>>>>>> of >>>>>>>>>>>> special >>>>>>>>>>>> function (if >>>>>>>>>>>> + any) that this function is. OPTYPE is the type >>>>>>>>>>>> given >>>>>>>>>>>> in >>>>>>>>>>>> a >>>>>>>>>>>> conversion >>>>>>>>>>>> operator declaration, or the class type for a >>>>>>>>>>>> constructor/destructor. >>>>>>>>>>>> Returns the actual return type of the >>>>>>>>>>>> function; >>>>>>>>>>>> that >>>>>>>>>>>> may >>>>>>>>>>>> be >>>>>>>>>>>> different >>>>>>>>>>>> than TYPE if an error occurs, or for certain >>>>>>>>>>>> special >>>>>>>>>>>> functions. >>>>>>>>>>>> */ >>>>>>>>>>>> @@ -12361,8 +12362,19 @@ check_special_function_return_type >>>>>>>>>>>> (special_function_kind sfk, >>>>>>>>>>>> tree type, >>>>>>>>>>>> tree optype, >>>>>>>>>>>> int >>>>>>>>>>>> type_quals, >>>>>>>>>>>> + const cp_declarator >>>>>>>>>>>> *declarator, >>>>>>>>>>>> + location_t id_loc, >>>>>>>>>>> >>>>>>>>>>> id_loc should be the same as declarator->id_loc? >>>>>>>>>> You’re right. >>>>>>>>>> >>>>>>>>>>>> const >>>>>>>>>>>> location_t* >>>>>>>>>>>> locations) >>>>>>>>>>>> { >>>>>>>>>>>> + /* If TYPE is unspecified, DECLARATOR, if set, should >>>>>>>>>>>> not >>>>>>>>>>>> represent a pointer >>>>>>>>>>>> + or a reference type. */ >>>>>>>>>>>> + if (type == NULL_TREE >>>>>>>>>>>> + && declarator >>>>>>>>>>>> + && (declarator->kind == cdk_pointer >>>>>>>>>>>> + || declarator->kind == cdk_reference)) >>>>>>>>>>>> + error_at (id_loc, "expected unqualified-id before >>>>>>>>>>>> %qs >>>>>>>>>>>> token", >>>>>>>>>>>> + declarator->kind == cdk_pointer ? "*" : >>>>>>>>>>>> "&"); >>>>>>>>>>> >>>>>>>>>>> ...and id_loc isn't the location of the ptr-operator, it's >>>>>>>>>>> the >> >>>>>>>>>>> location of the identifier, so this indicates the wrong >>>>>>>>>>> column. >>>>>>>>>>> I >>>>>>>>>>> think using declarator->id_loc makes sense, just not >>>>>>>>>>> pretending >>>>>>>>>>> it's >>>>>>>>>>> the location of the *. >>>>>>>>>> Good catch, thanks. >>>>>>>>>> >>>>>>>>>>> Let's give diagnostics more like the others later in the >>>>>>>>>>> function >>>>>>>>>>> instead of trying to emulate cp_parser_error. >>>>>>>>>> Makes sense. This is what the updated patch does, >>>>>>>>>> successfully >>>>>>>>>> tested on >>>>>>>>>> x86_64-pc-linux-gnu. OK for GCC 16? >>>>>>>>> >>>>>>>>> OK. >>>>>>>> >>>>>>>> Does this also fix 118304? If so, let's go ahead and apply it >>>>>>>> to >>>>>>>> GCC >>>>>>>> 15. >>>>>>> I have checked just now, and we still ICE for 118304’s >>>>>>> testcase >>>>>>> with >>>>>>> that fix. >>>>>> >>>>>> Why doesn't the preeexisting >>>>>> >>>>>> type = void_type_node; >>>>>> >>>>>> in check_special_function_return_type fix the return type and >>>>>> avoid >> >>>>>> the ICE? >>>> >>>>> We hit the gcc_assert at method.cc:3593, that Marek’s fix >> >>>>> bypasses. >>>> >>>> Yes, but why doesn't check_special_function_return_type prevent >>>> that? >>> >>> Ah, because we call it before walking the declarator. We need to >>> check again later, perhaps in grokfndecl, that the type is correct. >>> Perhaps instead of your patch. >> One “issue” with adding another check in or close to grokfndecl >> is >> that DECLARATOR will have “been moved to the ID”, and the fact >> that >> we had a CDK_POINTER kind is “lost”. We could obviously somehow >> propagate this information, but there might be something easier. > > The information isn't lost: it's now reflected in the (wrong) return > type. One place it would make sense to check would be > >> if (ctype && (sfk == sfk_constructor >> || sfk == sfk_destructor)) >> { >> /* We are within a class's scope. If our declarator >> name >> is the same as the class name, and we are defining >> >> a >> function, then it is a constructor/destructor, and >> therefore >> returns a void type. */ > > Here 'type' is still the return type, we haven't gotten to > build_function_type yet. That’s true. However, doesn’t it make sense to cram all the checks about the return type of special functions in check_special_function_return_type, and return an error if that return type is invalid? Since we’re in stage 4, I understand we want to have the “most localised” fixes possible. Would it work for you if I submitted a patch that adds the check where you mentioned for GCC 15 (fixing 108304 only), and another (similar to the one in my previous message) for GCC 16 (fixing both 108304 and 108306)? Or is there some more fundamental issue I’m missing with this approach? Thanks, Simon
