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

[Richard Smith]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141211/61868151/attachment.html>