Page MenuHomePhabricator

[OpenCL] Restrict address space conversions in nested pointers
ClosedPublic

Authored by Anastasia on Jan 24 2020, 8:14 AM.

Details

Summary

This patch is fixing the issue reported in:
http://lists.llvm.org/pipermail/cfe-dev/2020-January/064273.html
and in the bug:
https://bugs.llvm.org/show_bug.cgi?id=39674

Since address space conversion changes pointer representation it is not legal to do it for the nested pointers even with compatible address spaces. Because the address space conversion in the nested levels can't be generated. The behavior implemented by this patch is as follows:

  • Reject any implicit conversions of nested pointers with address spaces.
  • Reject address space of nested pointers conversions in safe casts e.g. const_cast or static_cast.
  • Allow conversion in low level C-style or reinterpret_cast but with a warning (this aligns with OpenCL C behavior).

Diff Detail

Event Timeline

Anastasia created this revision.Jan 24 2020, 8:14 AM
rjmccall requested changes to this revision.Jan 28 2020, 9:59 AM
rjmccall added inline comments.
clang/test/Misc/warning-flags.c
41 ↗(On Diff #240205)

Please give this warning a warning group rather than changing this test.

clang/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
521

Can we specialize the diagnostic here so that we get the good diagnostic in both language modes?

This revision now requires changes to proceed.Jan 28 2020, 9:59 AM

This patch looks good to me.

I do agree with John that it would be good to add the warning to a warning group.

Anastasia marked an inline comment as done.Jan 30 2020, 3:49 AM
Anastasia added inline comments.
clang/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
521

I am not sure of the approach so I am wondering if you have any good suggestions.

It seems in C++ Clang exits with quite generic Incompatible in the implicit conversion case because the functionality for doing the semantic checks doesn't generally set Sema::AssignConvertType.

https://clang.llvm.org/doxygen/SemaExpr_8cpp_source.html

Sema::CheckSingleAssignmentConstraints:


  if (getLangOpts().CPlusPlus) {
      if (!LHSType->isRecordType() && !LHSType->isAtomicType()) {
        // C++ 5.17p3: If the left operand is not of class type, the
        // expression is implicitly converted (C++ 4) to the
        // cv-unqualified type of the left operand.
        QualType RHSType = RHS.get()->getType();
        if (Diagnose) {
          RHS = PerformImplicitConversion(RHS.get(), LHSType.getUnqualifiedType(),
                                          AA_Assigning);
        } else {
          ...
        }
        if (RHS.isInvalid())
          return Incompatible;

However for example in IsStandardConversion I can see that Sema::AssignConvertType is being set but only for classifying the conversion sequence. It is never propagated outside.

http://clang.llvm.org/doxygen/SemaOverload_8cpp_source.html

IsStandardConversion:


    ExprResult ER = ExprResult{From};
    Sema::AssignConvertType Conv =
        S.CheckSingleAssignmentConstraints(ToType, ER,
                                           /*Diagnose=*/false,
                                           /*DiagnoseCFAudited=*/false,
                                           /*ConvertRHS=*/false);
    ImplicitConversionKind SecondConv;
    switch (Conv) {
    case Sema::Compatible:
      SecondConv = ICK_C_Only_Conversion;
      break;
    // For our purposes, discarding qualifiers is just as bad as using an
    // incompatible pointer. Note that an IncompatiblePointer conversion can drop
    // qualifiers, as well.
    case Sema::CompatiblePointerDiscardsQualifiers:
    case Sema::IncompatiblePointer:
    case Sema::IncompatiblePointerSign:
      SecondConv = ICK_Incompatible_Pointer_Conversion;
      break;
    default:
      return false;
    }

In most of other places it's not being used for C++ mode. I feel like C++ flow was not written to set the conversion type failure. Maybe there is something else I could use in C++ mode to specialize this diagnostic? Otherwise, it seems like quite a lot of Sema would have to be modified for C++ flow because implicit conversions are used in many different places.

Is there no follow-up code when actually emitting the failure diagnostic which tries to figure out a more specific cause of failure?

Is there no follow-up code when actually emitting the failure diagnostic which tries to figure out a more specific cause of failure?

After looking at this a bit more it seems C++ part calls the diagnostic code earlier:

Sema::PerformImplicitConversion:

   case ImplicitConversionSequence::BadConversion:
     bool Diagnosed =
         DiagnoseAssignmentResult(Incompatible, From->getExprLoc(), ToType,
                                  From->getType(), From, Action);

It just seems to always pass Incompatible. We could add follow up code before? This would be easier than modifying the existing flow completely. However, looking at this holistically we would need to repeat all checks classifying the failure types...

If it's reasonable to persist the failure kind, that would probably be good, but it might be a lot of work.

If it's reasonable to persist the failure kind, that would probably be good, but it might be a lot of work.

I have looked at it earlier and it indeed affects a lot of Sema in C++ mode. Besides amount of work I am not sure I understand the flow enough to come up with a good approach.

Maybe we can finalize this review first to fix the regression. Because I would like it to be ported to the release 10 branch too. Then I can spend some time to see if I can come up with something better for the diagnostic too. :)

Anastasia updated this revision to Diff 242312.Feb 4 2020, 5:49 AM

Add warning into IncompatiblePointerTypesDiscardsQualifiers group.

rjmccall accepted this revision.Feb 4 2020, 7:43 AM

Okay, we can go with this for now, since it does fix a bug.

This revision is now accepted and ready to land.Feb 4 2020, 7:43 AM

Okay, we can go with this for now, since it does fix a bug.

Thanks! Btw I uploaded straw-man patch for improving the diagnostics: https://reviews.llvm.org/D74116.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2020, 4:07 AM