rewriteBuiltinFunctionDecl incorrectly tests for address-space-qualified pointer operand.

[npjdesres]

Summary

Brief

There is a bug pertaining to Builtin functions with address-space-qualified pointer operands.

Context

clang::Sema::ActOnCallExpr visits every call expression to perform type checking and symbol table lookup. In doing so, it may call rewriteBuiltinFunctionDecl in order to specialize a non-address-space-qualified builtin function against the address-space qualifiers of a callsite's actual parameters.

Function rewriteBuiltinFunctionDecl compares the type of each formal parameter to the type of the corresponding actual parameter, in turn. If it detects a mismatch in the address spaces of pointer operands, it sets NeedsNewDecl=true and specializes the declaration accordingly.

Bug

The test for operand type mismatch is incorrect; specifically, it tests the address-space qualifiers of the pointer, rather than the pointee type. Instead of testing "ParamType.getQualifiers().hasAddressSpace() it should test "ParamType->getPointeeType().getQualifiers().hasAddressSpace()"

Pointer to (Address-Space-Qualified T) vs (Address-Space-Qualified Pointer) to T

Suppose that my target's builtin file (include/clang/Basic/BuiltinsXXX.def) declares this builtin with an address-space-qualfied pointer operand:

// v4i32 __builtin_FOO(volatile addrspace(109) v4i32 *a, v4i32 b)
BUILTIN(__builtin_FOO, "V4iV4iD*109V4i", "n")

In the pre-patch system, I can observe the parameter type by placing a breakpoint on the line that sets NeedsNewDecl=true, and then dumping the parameter type:

$ cat testcase.cpp
typedef int __attribute__((vector_size(4*sizeof(int)))) v4i32;

void test() {
  v4i32 volatile __attribute__((address_space(109))) *A;
  v4i32 B;
  __builtin_FOO( A, B );
}

$ gdb --args clang++ testcase.cpp
(gdb) set follow-fork-mode child
(gdb) b SemaExpr.cpp:4981
(gdb) r
Breakpoint 1
(gdb) p ParamType.dump()
PointerType 0x6859c30 '__attribute__((__vector_size__(4 * sizeof(int)))) int volatile __dispatch *'
`-QualType 0x6859c1c '__attribute__((__vector_size__(4 * sizeof(int)))) int volatile __dispatch' volatile __dispatch
  `-VectorType 0x68596e0 '__attribute__((__vector_size__(4 * sizeof(int)))) int' 4
    `-BuiltinType 0x68590c0 'int'

(Where "__dispatch" is my target's name for address-space 109).

This dump demonstrates that the address space qualifier is on the pointee, not the pointer. The parameter is a of type "Pointer to (Address-Space-Qualified T)" rather than "(Address-Space-Qualified Pointer) to T."

[rsmith]

Is it possible to observe this for any of our in-tree targets? If so, can you add a unit test?

[npjdesres]

I should also mention that my target works fine in releases 3.5 and 3.6, but breaks now in 3.7. The critical difference was the introduction rewriteBuiltinFunctionDecl.

Specfiically, although rewriteBuiltinFunctionDel creates a new function declaration with name and type similar to the builtin, the new declaration is NOT a builtin because the resulting declaration does not have a builtin ID, i.e., FDecl->getBuiltinID() now returns zero. Subsequent hooks, in particular those in CodeGenFunction::emitTargetBuiltinExpr are no longer able to select the builtin according to the builtin ID.

Without the BuiltinID, I'm not sure how my backend should implement CodeGenFunction::emitTargetBuiltinExpr. Should it strcmp the name of every called function against a list of target builtins?

It will not be easy to expose a testcase using only in-tree targets. NVPTX is the only in-tree target that exposes builtins with address-space-qualified pointer arguments (builtins "__nvvm_atom_*_{g,s}_i" accept first parameter pointer to address space 1 or 3, respectively), and even then the NVPTX backend does not actually implement them in CodeGenFunction::EmitNVPTXBuiltinExpr.

[rsmith]

OK, if this isn't visible for any in-tree target, then I suppose we can go ahead without a test on the basis that this is obviously a correct fix. Do you need someone to commit for you?

[npjdesres]

Thanks. I do not have commit access. Could you please commit it for me?

[npjdesres]

This patch has been approved but not yet committed. Is there anything else I need to do?

[npjdesres]

Ping. Can someone please commit this diff?

[Eugene.Zelenko]

Looks like patch was not committed.

[npjdesres]

You are correct. I'm still waiting for someone/anyone with commit to put this in.

lib/Sema/SemaExpr.cpp

static FunctionDecl *rewriteBuiltinFunctionDecl(Sema *Sema, ASTContext &Context,
   SmallVector<QualType, 8> OverloadParams;
 
   for (QualType ParamType : FT->param_types()) {
 
     // Convert array arguments to pointer to simplify type lookup.
     Expr *Arg = Sema->DefaultFunctionArrayLvalueConversion(ArgExprs[i++]).get();
     QualType ArgType = Arg->getType();
     if (!ParamType->isPointerType() ||
-        ParamType.getQualifiers().hasAddressSpace() ||
+        ParamType->getPointeeType().getQualifiers().hasAddressSpace() ||
         !ArgType->isPointerType() ||
         !ArgType->getPointeeType().getQualifiers().hasAddressSpace()) {
       OverloadParams.push_back(ParamType);
       continue;
     }
 
     NeedsNewDecl = true;
     unsigned AS = ArgType->getPointeeType().getQualifiers().getAddressSpace();