r223852 - AST: Don't assume two zero sized objects live at different addresses

[David Majnemer]

On Thu, Dec 11, 2014 at 7:08 PM, Richard Smith <richard at metafoo.co.uk>
wrote:

> On Thu, Dec 11, 2014 at 6:50 PM, David Majnemer <david.majnemer at gmail.com>
> wrote:
>
>> On Thu, Dec 11, 2014 at 1:12 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>>
>>> On Thu, Dec 11, 2014 at 1:02 PM, David Majnemer <
>>> david.majnemer at gmail.com> wrote:
>>>
>>>> On Thu, Dec 11, 2014 at 12:45 PM, Richard Smith <richard at metafoo.co.uk>
>>>> wrote:
>>>>
>>>>> On Thu, Dec 11, 2014 at 11:47 AM, David Majnemer <
>>>>> david.majnemer at gmail.com> wrote:
>>>>>
>>>>>> On Thu, Dec 11, 2014 at 11:28 AM, Richard Smith <
>>>>>> richard at metafoo.co.uk> wrote:
>>>>>>
>>>>>>> On Tue, Dec 9, 2014 at 3:32 PM, David Majnemer <
>>>>>>> david.majnemer at gmail.com> wrote:
>>>>>>>
>>>>>>>> Author: majnemer
>>>>>>>> Date: Tue Dec  9 17:32:34 2014
>>>>>>>> New Revision: 223852
>>>>>>>>
>>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=223852&view=rev
>>>>>>>> Log:
>>>>>>>> AST: Don't assume two zero sized objects live at different addresses
>>>>>>>>
>>>>>>>> Zero sized objects may overlap with each other or any other object.
>>>>>>>>
>>>>>>>> This fixes PR21786.
>>>>>>>>
>>>>>>>> Modified:
>>>>>>>>     cfe/trunk/lib/AST/ExprConstant.cpp
>>>>>>>>     cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp
>>>>>>>>
>>>>>>>> Modified: cfe/trunk/lib/AST/ExprConstant.cpp
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=223852&r1=223851&r2=223852&view=diff
>>>>>>>>
>>>>>>>> ==============================================================================
>>>>>>>> --- cfe/trunk/lib/AST/ExprConstant.cpp (original)
>>>>>>>> +++ cfe/trunk/lib/AST/ExprConstant.cpp Tue Dec  9 17:32:34 2014
>>>>>>>> @@ -1422,6 +1422,12 @@ static bool IsWeakLValue(const LValue &V
>>>>>>>>    return Decl && Decl->isWeak();
>>>>>>>>  }
>>>>>>>>
>>>>>>>> +static bool isZeroSized(const LValue &Value) {
>>>>>>>> +  const ValueDecl *Decl = GetLValueBaseDecl(Value);
>>>>>>>> +  return Decl && isa<VarDecl>(Decl) &&
>>>>>>>> +         Decl->getASTContext().getTypeSize(Decl->getType()) == 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>  static bool EvalPointerValueAsBool(const APValue &Value, bool
>>>>>>>> &Result) {
>>>>>>>>    // A null base expression indicates a null pointer.  These are
>>>>>>>> always
>>>>>>>>    // evaluatable, and they are false unless the offset is zero.
>>>>>>>> @@ -6979,6 +6985,10 @@ bool IntExprEvaluator::VisitBinaryOperat
>>>>>>>>              (RHSValue.Base && RHSValue.Offset.isZero() &&
>>>>>>>>               isOnePastTheEndOfCompleteObject(Info.Ctx, LHSValue)))
>>>>>>>>            return Error(E);
>>>>>>>> +        // We can't tell whether an object is at the same address
>>>>>>>> as another
>>>>>>>> +        // zero sized object.
>>>>>>>> +        if (isZeroSized(LHSValue) || isZeroSized(RHSValue))
>>>>>>>> +          return Error(E);
>>>>>>>>
>>>>>>>
>>>>>>> We can do better here: one of the pointers must be to a zero-sized
>>>>>>> object, and the other must be a past-the-end pointer (where a pointer to a
>>>>>>> zero-sized object is considered to be a past-the-end pointer).
>>>>>>>
>>>>>>
>>>>>> Ah, clever.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>          // Pointers with different bases cannot represent the same
>>>>>>>> object.
>>>>>>>>          // (Note that clang defaults to -fmerge-all-constants,
>>>>>>>> which can
>>>>>>>>          // lead to inconsistent results for comparisons involving
>>>>>>>> the address
>>>>>>>>
>>>>>>>> Modified: cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp?rev=223852&r1=223851&r2=223852&view=diff
>>>>>>>>
>>>>>>>> ==============================================================================
>>>>>>>> --- cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp (original)
>>>>>>>> +++ cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp Tue Dec  9
>>>>>>>> 17:32:34 2014
>>>>>>>> @@ -1955,3 +1955,9 @@ namespace EmptyClass {
>>>>>>>>    constexpr E2 e2b(e2); // expected-error {{constant expression}}
>>>>>>>> expected-note{{read of non-const}} expected-note {{in call}}
>>>>>>>>    constexpr E3 e3b(e3);
>>>>>>>>  }
>>>>>>>> +
>>>>>>>> +namespace PR21786 {
>>>>>>>> +  extern void (*start[])();
>>>>>>>> +  extern void (*end[])();
>>>>>>>> +  static_assert(&start != &end, ""); // expected-error {{constant
>>>>>>>> expression}}
>>>>>>>> +}
>>>>>>>>
>>>>>>>
>>>>>>> This testcase looks like valid C++ code to me; the comparison is a
>>>>>>> constant expression under the C++ rules and evaluates to true. I don't
>>>>>>> think we can apply this check in this case, only when we have a complete
>>>>>>> type that is zero-sized. That means we'll constant-fold equality
>>>>>>> comparisons to 'false' even if they turn out to be true, but that seems to
>>>>>>> be unavoidable.
>>>>>>>
>>>>>>
>>>>>> I don't quite understand why we should fold that comparison to false,
>>>>>> GCC and ICC both consider that expression to be non-constant.
>>>>>>
>>>>>
>>>>> That doesn't make them right. =) C++ does not have zero-sized types,
>>>>> nor the possibility of the above objects being at the same address. Per its
>>>>> constant evaluation rules, the above expression *is* a constant expression,
>>>>> and we are required to treat it as such. In this regard, zero-sized types
>>>>> are not a conforming extension.
>>>>>
>>>>
>>>> They are both (potentially) one-past-the-end objects though.  I think
>>>> our hands are tied, seeing as how we use the constant expression evaluation
>>>> to CodeGen if conditions and what-not.
>>>>
>>>
>>> I don't think it's so clear. No valid C or C++ program can have an array
>>> of zero bound, and I think we should generally prioritize doing the right
>>> thing on conforming code over giving better semantics to a language
>>> extension. I think the question is, does any real code rely on this not
>>> being constant-folded for incomplete arrays that turn out to have a bound
>>> of zero?
>>>
>>
>> I'm not entirely sure how we can answer that but I found the following
>> after a minute of digging around the linux kernel:
>>
>> kernel_memsize = kernel_size + (_end - _edata);
>>
>> _end and _edata are two linker generated symbols.  If people are
>> subtracting these things, I can imagine that they are also comparing them.
>>
>>
>>>
>>> In any case, the incomplete-type case should be restricted to incomplete
>>> arrays, since incomplete class types can never have zero size in C++.
>>>
>>
>> I completely agree. In an ideal world, I'd stuff this zero-sized
>> mumbo-jumbo under a hypothetical -fgcc-compatibility (or something similar).
>>
>
> OK, so I think the compromise position is:
>
> An entity is considered as being possibly-zero-sized if either:
> 1) The type is incomplete and we're in C, or
> 2) The type is an array of unknown bound and we're in C++, or
> 3) The type is complete and its size is zero.
>
> We refuse to constant-fold an address comparison if one operand is
> possibly-zero-sized, and the other is either possibly-zero-sized or
> evaluates to the address of the start or end of an object of a complete
> type.
>
> Does that make sense to you? I think that's as close as we can get to the
> standard behavior in C and C++ without miscompiling address comparisons
> against zero-sized objects.
>

I think this makes sense.  I don't think we want to enshrine that struct S
{ int x[0]; }; will reliably give you a zero sized type in C++ (it doesn't
in the MS ABI, it has size four) which is exactly what point #2 prevents.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141211/8371f44f/attachment.html>