Page MenuHomePhabricator

[Sema][SVE] Reject sizeof and alignof for sizeless types
ClosedPublic

Authored by rsandifo-arm on Mar 3 2020, 2:40 PM.

Details

Summary

clang current accepts:

void foo1(__SVInt8_t *x, __SVInt8_t *y) { *x = *y; }
void foo2(__SVInt8_t *x, __SVInt8_t *y) {
  memcpy(y, x, sizeof(__SVInt8_t));
}

The first function is valid ACLE code and generates correct LLVM IR.
The second function is invalid ACLE code and generates a zero-length
memcpy.

The point of this patch is to reject the use of sizeof in the second
case, rather than silently generating incorrect code.

There's no similar wrong-code bug for alignof. However, the SVE ACLE
conservatively treats alignof in the same way as sizeof, just as the
C++ standard does for incomplete types. The idea is that layout of
sizeless types is an implementation property and isn't defined at
the language level.

(We could relax the alignof rules in future if they turn out to be
too restrictive. It would be harder to go the other way though,
and forbid alignof after previously treating it as valid.)

Implementation-wise, the patch adds a new CompleteTypeKind enum
that controls whether RequireCompleteType & friends accept sizeless
built-in types. For now the default is to maintain the status quo
and accept sizeless types. However, the end of the series will flip
the default and remove the Default enum value.

The patch also adds new ...CompleteSized... wrappers that callers can
use if they explicitly want to reject sizeless types. The callers then
use diagnostics that have an extra 0/1 parameter to indicats whether
the type is sizeless or not.

The idea is to have three cases:

  1. calls that explicitly reject sizeless types, with a tweaked diagnostic for the sizeless case
  1. calls that explicitly allow sizeless types
  1. normal/old-style calls that don't make an explicit choice either way

Once the default is flipped, the 3. calls will conservatively reject
sizeless types, using the same diagnostic as for other incomplete types.

Diff Detail

Event Timeline

rsandifo-arm created this revision.Mar 3 2020, 2:40 PM
Herald added a project: Restricted Project. · View Herald Transcript

The planned changes to RequireCompleteType should catch this, right? If we want a specialized diagnostic for sizeless types in RequireCompleteType, we should probably just pass it to RequireCompleteType. (Otherwise, you'll end up with the "wrong" diagnostic in templates.)

Just looked at the comment on D75572; not sure if RequireCompleteType is going to reject sizeless types. In any case, you have to instantiate templates using RequireCompleteType before you can determine whether a type is sizeless.

The planned changes to RequireCompleteType should catch this, right? If we want a specialized diagnostic for sizeless types in RequireCompleteType, we should probably just pass it to RequireCompleteType. (Otherwise, you'll end up with the "wrong" diagnostic in templates.)

Yeah, the planned changes to RequireCompleteType would catch this by default. If RequireCompleteType should always get the first shot, I guess there are three possible approaches:

  1. Stick with the approach in D62962, without specialised error messages. I can break that patch up into smaller chunks if the general approach seems right.
  2. Stick with the approach in this patch of having seperate isSizelessType checks and diagnostics, but test isSizelessType after RequireCompleteType instead of before it. The later changes to RequireCompleteType allow sizeless types to be let through on demand, and the isSizelessType handling would be similar to the isFunctionType handling just below.
  3. Like you say, stick with separate diagnostics for incomplete and sizeless types but make RequireCompleteType emit them both.

I guess one way doing 3. would be to use %select{incomplete|sizeless} and pass the result of isSizelessType() as one of the diagnostic parameters to RequireCompleteType. But in some ways that feels a bit clunky because the caller is then doing the work to prove that the use is invalid (which it always is if isSizelessType()) but isn:t actually doing the work to emit the associated diagnostic. Another option would be to make RequireCompleteType have a choice between three actions:

  • the normal, standard-defined behaviour, with no special error messages for sizeless types. This would remain the default if no action is explicitly specified.
  • a mode that explicitly allows sizeless types
  • a mode that explicitly disallows sizeless types, with the first diagnostic parameter being whether the type is sizeless or not. In contrast to the above, RequireCompleteType would add this parameter automatically.

I think the specialized error messages are useful. I don't have a strong opinion on what order the patches should land.

The difference between emitting the diagnostic in RequireCompleteType, vs. letting the caller do it, is basically just that it's harder to forget to check whether the type is sizeless. I think that's important enough to be worth complicating the API a bit; better to forbid scalable types, rather than crash or miscompile later. Probably the entry point that allows sizeless types should have a different name.

Changes in v2:

  • Emit the diagnostic from RequireCompleteTypeImpl instead of checking for sizeless types separately. This implements option 3. from the earlier discussion.
  • Reformat the patch using git-clang-format.
rsandifo-arm edited the summary of this revision. (Show Details)Mar 4 2020, 9:36 AM

I think the specialized error messages are useful. I don't have a strong opinion on what order the patches should land.

OK. The new version keeps the tailored diagnostic but emits it from RequireCompleteTypeImpl instead.

The difference between emitting the diagnostic in RequireCompleteType, vs. letting the caller do it, is basically just that it's harder to forget to check whether the type is sizeless. I think that's important enough to be worth complicating the API a bit; better to forbid scalable types, rather than crash or miscompile later. Probably the entry point that allows sizeless types should have a different name.

Yeah, the plan is to make RequireCompleteType reject sizeless types unless the caller has explicitly said otherwise. We want to do that whatever approach is taken for emitting the diagnostics. However, starting off with a patch that makes the types incomplete and then gradually relaxing the rules would interfere too much with other people's work, so the idea was to do it the other way around. By building the series up this way, the final patch that makes sizeless types incomplete ends up being much smaller (and hopefully more reviewable!) than D62962 was.

efriedma accepted this revision.Mar 4 2020, 4:02 PM
efriedma added a reviewer: rsmith.

LGTM

Please give it a day or two before you merge in case someone else has an opinion on the new API.

This revision is now accepted and ready to land.Mar 4 2020, 4:02 PM
This revision was automatically updated to reflect the committed changes.