[PATCH] guard against calls with fewer arguments than the function type implies (PR 95581)

[Richard Biener]

On Tue, Jun 9, 2020 at 5:51 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 6/9/20 1:43 AM, Richard Biener wrote:
> > On Mon, Jun 8, 2020 at 11:59 PM Martin Sebor via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> PR 95581 reports an ICE in a call to gimple_call_arg() with
> >> an out-of-bounds argument number (2).  The attached patch avoids
> >> the ICE but I'm not sure it's the right fix.  Other than verifying
> >> the ICE is gone with a powerpc64 cross-compiler I couldn't come up
> >> with a generic test cases to exercise this change.
> >>
> >> The call, synthesized by the vectorizer, is
> >>
> >>     vect__5.6_24 = __builtin_altivec_mask_for_load (vectp_a.5_8);
> >>
> >> but the function is declared by the powerpc64 back end to take two
> >> arguments: long and const void*.  Is it correct for the builtin to
> >> be called with fewer arguments than their type indicates?  (If it
> >> isn't the patch shouldn't be necessary but some other fix would
> >> be needed.)
> >
> > I think the backend declaration is wrong, the function only takes
> > a void * argument and returns a long.
>
> Thanks.  In his comment on the bug Segher (CC'd) points to
> the internals manual that documents the function:
>
> https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/target.def;h=07059a87caf7cc0237d583124b476ee45ea41ed5;hb=HEAD#l1744
>
> (By the way, thanks for the pointer!)
>
> If I read it right, ihe function f in
> the TARGET_VECTORIZE_BUILTIN_MASK_FOR_LOAD documentation is
> __builtin_altivec_mask_for_load.  Although the manual isn't
> unequivocal about this but it does suggest the address addr
> the function is given as an argument should be the first
> (and presumably only) argument.  This matches the call in
> the IL where the first argument is a pointer, but not
> the function's signature.
>
> I think the middle end needs to be able to rely on built-in
> function types when processing calls: i.e., the types and
> numbers of actual arguments in the calls need to match those
> of the built-in function type.  Otherwise it would have to
> validate every call against the function signature and avoid
> treating it as a built-in if it doesn't match.  There are
> parts of the middle end that do that for library built-ins
> (because they can be declared in a program with mismatched
> signatures) but as we have seen it's error-prone.  I don't
> think it would be helpful to try to extend this approach to
> internal built-ins.
>
> So I agree that the real problem is the declaration of
> the built-in.
>
> I have no issue with also committing the proposed patch
> in the meantime, until the back end is fixed, if it helps.
> But I'll leave that decision to the middle end maintainers.

The backend needs to be fixed.

Richard.

> Martin