Re: [Xen-devel] [PATCH] x86/msr: Fix fallout from mostly c/s 832c180

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.04.19 at 15:52, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 12/04/2019 11:46, Jan Beulich wrote:
>>
>>>  The function promising not to alter the pointed-to-object
>>> includes the entire child callgraph.
>>>
>>>
>>> The code you insisted Paul to add is:
>>>
>>> struct vcpu *v = cv->domain->vcpu[cv->vcpu_id];
>>>
>>> which is identical to:
>>>
>>> struct vcpu *v = (struct vcpu *)cv;
>> It is not identical; it is having the same effective behavior when
>> compiled with today's compilers.
>>
>>> Which highlights very clearly that this function has undefined behaviour.
>> It doesn't, no.
> 
> Yes it literally does, and even in the very first sentence you quoted.
> 
> Reproduced here:
> 
>> "If an attempt is made to modify an object defined with a const-
>>  qualified type through use of an lvalue with non-const-qualified
>>  type, the behavior is undefined.
> 
> There is exactly one object for this vcpu.
> 
> *cv, as defined by the prototype, is const qualified, and is this object.
> 
> *v is the same object, and mutates it.

But these are _declarations_, not _definitions_.

> C doesn't necessarily know that "cv->domain->vcpu[cv->vcpu_id] == cv",
> but it really is an alias in practice, and therefore is UB under that rule.

As per above, this rule makes it UB only if the object _definition_
was (effectively, as per the footnote) const.

>>> An optimising compiler which uses an object, and passes a const pointer
>>> to that object to a function, is permitted to retain assumptions derived
>>> from that state across the function call sequence point, because the ABI
>>> of the function states that the content of the object doesn't change.
>> Very much not so, no. Take this simple (and granted contrived)
>> example:
>>
>> int integer;
>>
>> int test(void) {
>>      func(&integer);
>>      return integer;
>> }
>>
>> and in a different CU (just to avoid the effect of the compiler
>> inlining the whole thing)
>>
>> void func(const int*pi) {
>>      integer = ~*pi;
>> }
>>
>> Various other examples are possible, including ones where
>> there's nothing contrived at all.
> 
> How about a concrete example which matches the code pattern under
> argument and demonstrates the issue.
> 
> void func(const int *pi)
> {
>     int *i = (int *)pi;

To give a valid example, you need to get away without casting
away const-ness. This is the real difference between what you
correctly name UB and what I had suggested while reviewing
Paul's patch.

>     *i = 6;
> }
> 
> And in a separate translation unit.
> 
> int test(void)
> {
>     const int i = 4;
> 
>     func(&i);
>     assert(i == 4);
> 
>     return i;
> }
> 
> Funnily enough, the assert never triggers.  Even at -O0, it never gets
> compiled in and test has its return value in the form `mov $4, %eax;
> ret`, and the only way that occurs is because of the UB.

Sure - even without any optimization the compiler sees the const
on the _definition_ of i.

>>> But if you'd prefer a different argument, how about a contradiction.
>>>
>>> By your interpretation, the const keyword is utterly useless because a
>>> compiler must treat all const pointers as non-const, because the
>>> pointed-to object can change in any arbitrary way at any point.  If this
>>> were the intended interpretation, const would never have been added to
>>> the C language because it would waste space in the compiler for 0 gain.
>>>
>>> The fact it was added demonstrates that it had real material gains,
>>> which means it isn't a useless keyword, which means the compiler really
>>> may depend on the content of a const pointed-to-object not changing at all.
>> I doubt this, and you provide no source where you take from that
>> this was the intention. And despite what you say, "const" has its
>> value nevertheless - it allows the compiler to tell you when a piece
>> of code modifies an object that you didn't mean to alter.
>>
>> Quote from the language spec:
>>
>> "If an attempt is made to modify an object defined with a const-
>>  qualified type through use of an lvalue with non-const-qualified
>>  type, the behavior is undefined. If an attempt is made to refer
>>  to an object defined with a volatile-qualified type through use
>>  of an lvalue with non-volatile-qualified type, the behavior is
>>  undefined."
>>
>> And the respective footnote:
>>
>> "This applies to those objects that behave as if they were
>>  defined with qualified types, even if they are never actually
>>  defined as objects in the program (such as an object at a
>>  memory-mapped input/output address)."
>>
>> Throughout the verb used is "defined", not "declared". If any
>> struct vcpu instance actually lived in .rodata (for example),
>> then (without casting away constness) it would be impossible to
>> construct a non-const pointer to it. Hence there would be no
>> legitimate means to create a way to modify that instance. But
>> that's specifically not the case here (or in the example given).
> 
> A function which takes a const vcpu* does not know, and has no way of
> proving, that the object really wasn't const.

Exactly. Which is why the compiler can't derive anything for its
own code generation purposes.

> I will admit that I made a made a mistake with the optimisation claim. 
> The outer function, because it can't see the declaration of the object
> itself, also can't assume there aren't other aliases.

And this is true throughout the code, not just for the outer
function.

> But none of this stops the casting away of const being UB, and it still
> remains completely dishonest programming to declare
> vmx_set_guest_bndcfgs() as taking a const vcpu, and then modifying it.

Well, I was aware that the construct might not be liked, and hence
I had specifically called for opinions. Nevertheless it's not at all
"dishonest programming" to me. Quite the inverse - I think the VMCS
enter/exit logic should have been written this way from the beginning.
After all it's not the first time that because of its internal workings
something else can get const added.

And btw, C++ specifically has the "mutable" keyword to override
const in certain (special) cases.

I can accept though that you and maybe others don't agree with
my view here, and hence (as expressed before) I'm not insisting
on it staying the way it is right now.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel