Understanding 'volatile' in lstrlib.cpp (Lua 5.3)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Understanding 'volatile' in lstrlib.cpp (Lua 5.3)

John W
Hello,

There are a few uses of the 'volatile' keyword in lstrlib.c, and I'm
trying to understand their purpose (lua 5.3).

I care most about the two instances of "volatile Ftypes u;" The other
one is copywithendian.

From what I understand, most uses of 'volatile' in Lua are protection
in the case of setjmp/longjmp stomping registers. Do those concerns
apply here?

These were introduced with commit
c172a4f7c2d452035f9cbb4285f2e6761ab7a68b, with the comment:
    'dumpint' and related functions replaced by 'string.pack'/'string.unpack'

So, not a lot of info there.

Are there any other concerns in this code that require 'volatile'? Or
maybe it's a workaround for some old compilers, such as mentioned in
the comments for 'aux_close' ?


For the extra-curious, this is a stumbling block for me as I implement
a custom lua_Number type, compiling it as C++ with operator
overloading to make my type behave much like a float. I made a
StackOverflow question with details here, if anyone is interested:
https://stackoverflow.com/questions/64071982/custom-number-class-can-it-be-made-to-work-in-place-of-float-or-other-buil

Thanks
-John
Reply | Threaded
Open this post in threaded view
|

Re: Understanding 'volatile' in lstrlib.cpp (Lua 5.3)

Viacheslav Usov
On Sat, Sep 26, 2020 at 12:57 AM John W <[hidden email]> wrote:

> Are there any other concerns in this code that require 'volatile'? Or
> maybe it's a workaround for some old compilers, such as mentioned in
> the comments for 'aux_close' ?

This is possible, but probably not the same bug.

The precision of floating point computations per the C standard is not
fully specified . For example, extended precision 80-bit registers may
be used when they are available even though the float and double types
might be 32 and 64 bit wide, respectively. Overly zealous optimizers
may optimize away memory stores (spills) and end up copying part of
the register, i.e., garbage, or just random garbage to begin with. The
volatile forces a memory store with a valid bit width and format.

Just guessing.

Cheers,
V..
Reply | Threaded
Open this post in threaded view
|

Re: Understanding 'volatile' in lstrlib.cpp (Lua 5.3)

Roberto Ierusalimschy
In reply to this post by John W
> There are a few uses of the 'volatile' keyword in lstrlib.c, and I'm
> trying to understand their purpose (lua 5.3).
>
> I care most about the two instances of "volatile Ftypes u;" The other
> one is copywithendian.

In ANSI C, you cannot assign to a field in an union and read from
another field, which is exactly the purpose of Ftypes.  The "volatile"
avoids optimizations based on that knowledge, ensuring that the
code executes "as expected": A write to a field actually writes
to that field, and a subsequent read from another field actually
reads from that field.

The ones in 'copywithendian' are to ensure compatibility with
the arguments. Otherwise, we have this kind of warning:

    lstrlib.c:1594:30: warning: passing argument 2 of ‘copywithendian’ discards ‘volatile’ qualifier from pointer target type [-Wdiscarded-qualifiers]
             copywithendian(buff, u.buff, size, h.islittle);

-- Roberto
Reply | Threaded
Open this post in threaded view
|

Re: Understanding 'volatile' in lstrlib.cpp (Lua 5.3)

Andrew Gierth
>>>>> "Roberto" == Roberto Ierusalimschy <[hidden email]> writes:

 Roberto> In ANSI C, you cannot assign to a field in an union and read
 Roberto> from another field, which is exactly the purpose of Ftypes.
 Roberto> The "volatile" avoids optimizations based on that knowledge,
 Roberto> ensuring that the code executes "as expected"

We don't find this qualifier to be necessary in PostgreSQL, which uses
exactly this trick when storing float values into the generic Datum type
(which is declared as uintptr_t) and vice-versa:

https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/include/postgres.h;h=c48f47e930ad05d3ae2e24b0c0c85662cd058a7b;hb=HEAD#l693

static inline float8
DatumGetFloat8(Datum X)
{
    union
    {
        int64       value;
        float8      retval;
    } myunion;

    myunion.value = DatumGetInt64(X);
    return myunion.retval;
}

static inline Datum
Float8GetDatum(float8 X)
{
    union
    {
        float8      value;
        int64       retval;
    } myunion;
    myunion.value = X;
    return Int64GetDatum(myunion.retval);
}

Both of these compile (on amd64 with any modern compiler) to simple move
instructions. For more strictness we could have used uint64, but in
practice we don't support platforms where ints are not 2s-complement so
this is moot.

The spec allows for the possibility of such conversions to generate
"trap representations" of the destination type (for example
reinterpreting an integer as a float might result in a signalling NaN,
with an exception raised if the value is ever used). But it explicitly
_does not_ declare this to be undefined behavior; what it says is that
the value of the stored type is reinterpreted as being of the accessed
type. Note especially that the type 'unsigned char' is not allowed to
have trap representations and that accessing a trap representation of
any type via lvalues of character type is _not_ undefined behavior.

Incidentally, the spec also explicitly makes it legal to access the
representation of any object by aliasing it as a character type. So it
is legal for example to use memcpy(charptr, &floatvar, sizeof(floatvar))
to copy the representation (and modern compilers will generally optimize
this to a single move instruction too).

In short, the qualifier is not required either by the spec or (as far as
I can determine) actual compilers; if you have an example where a
problem actually occurred with this I would be interested to know
details.

The volatile qualifier likely heavily pessimizes the code here, because
it forces the copywithendian loop to actually go byte-by-byte to memory,
which will cause a delay due to failure of store-forwarding; whereas
without a volatile qualifier the compiler will usually optimize it to a
byteswap instruction (where available) or a vectorized shuffle.

For example, on clang 10, amd64, -O2, this code:

#include <stdbool.h>
#include <stddef.h>
#include <string.h>

static void copy_endian(unsigned char *dst,
                        unsigned char *src,
                        size_t sz,
                        bool swap)
{
    if (swap)
        for (dst += sz; sz > 0; --sz)
            *--dst = *src++;
    else
        memcpy(dst, src, sz);
}
void emit_dbl(unsigned char *ptr, double d, bool swap)
{
    copy_endian(ptr, (unsigned char *) &d, sizeof(d), swap);
}

compiles to this:

emit_dbl:                               # @emit_dbl
        test    sil, sil
        je      .LBB1_2
        movq    rax, xmm0
        bswap   rax
        mov     qword ptr [rdi], rax
        ret
.LBB1_2:
        movsd   qword ptr [rdi], xmm0
        ret

--
Andrew.
Reply | Threaded
Open this post in threaded view
|

Re: Understanding 'volatile' in lstrlib.cpp (Lua 5.3)

Viacheslav Usov
On Thu, Oct 1, 2020 at 8:50 AM Andrew Gierth
<[hidden email]> wrote:

> The spec allows [...]

Note that the original claim was about "ANSI C", i.e., C89. Where
union-based type punning is implementation-defined.

On the other hand, I do not think that adding volatile would change
that, i.e., make it standard rather than implementation-defined.

Moreover, in C++ union-based type punning is undefined behavior, and
since the Lua source code intends to be consumable as C++, this is a
problem that volatile does not correctly address, either.

I believe memcpy() is the only way that would be truly portable with
all those things considered.

The code in str_unpack() might look like:

      case Kfloat: {
        char buff[sizeof (double)]; /* instead of volatile Ftypes u; */
        lua_Number num;
        copywithendian(buff, data + pos, size, h.islittle);
        if (size == sizeof(u.f)) { float f; memcpy(&f, buff, sizeof
f); num = (lua_Number)f; }
        else if (size == sizeof(u.d)) { double d; memcpy(&d, buff,
sizeof d); num = (lua_Number)d; }
        else memcpy(&num, buff, sizeof num);
        lua_pushnumber(L, num);
        break;
      }

Cheers,
V.
Reply | Threaded
Open this post in threaded view
|

Re: Understanding 'volatile' in lstrlib.cpp (Lua 5.3)

Andrew Gierth
>>>>> "Viacheslav" == Viacheslav Usov <[hidden email]> writes:

 Viacheslav> Moreover, in C++ union-based type punning is undefined
 Viacheslav> behavior, and since the Lua source code intends to be
 Viacheslav> consumable as C++, this is a problem that volatile does not
 Viacheslav> correctly address, either.

 Viacheslav> I believe memcpy() is the only way that would be truly
 Viacheslav> portable with all those things considered.

I think you are correct, though I'd be surprised if any C++ compiler did
the wrong thing with union type punning of basic types.

The C++ standard seems to have the same exception for aliasing a value
by way of a char or unsigned char type that C has. So your example has
one more copy than is needed - the bytes can be copied directly from or
to the "double" var without needing the intermediate char buff[].

--
Andrew.
Reply | Threaded
Open this post in threaded view
|

Re: Understanding 'volatile' in lstrlib.cpp (Lua 5.3)

Diego Nehab-4
I think memcpy is the safest approach. Fortunately, a good compiler is smart enough to optimize away the memcpy when it knows the object is aligned. GCC even has a built-in to let you tell the compiler about the alignment when it is not obvious. 

On Thu, Oct 1, 2020 at 06:07 Andrew Gierth <[hidden email]> wrote:
>>>>> "Viacheslav" == Viacheslav Usov <[hidden email]> writes:



 Viacheslav> Moreover, in C++ union-based type punning is undefined

 Viacheslav> behavior, and since the Lua source code intends to be

 Viacheslav> consumable as C++, this is a problem that volatile does not

 Viacheslav> correctly address, either.



 Viacheslav> I believe memcpy() is the only way that would be truly

 Viacheslav> portable with all those things considered.



I think you are correct, though I'd be surprised if any C++ compiler did

the wrong thing with union type punning of basic types.



The C++ standard seems to have the same exception for aliasing a value

by way of a char or unsigned char type that C has. So your example has

one more copy than is needed - the bytes can be copied directly from or

to the "double" var without needing the intermediate char buff[].



--

Andrew.



--
Sent from my phone
Reply | Threaded
Open this post in threaded view
|

Re: Understanding 'volatile' in lstrlib.cpp (Lua 5.3)

John W
Just another data point, since I recently saw it:
The well-used GLM library (which is C++) uses type punning through a
union just about as clear as can be:
    https://github.com/g-truc/glm/blob/b3f87720261d623986f164b2a7f6a0a938430271/glm/detail/func_common.inl#L722

I suppose, since it's C++ (not C90), it is technically undefined
behavior. But I have certainly worked on dozens of systems over
decades that rely on it, so it seems like extremely reliable undefined
behavior, as far as that goes (:  And of course, it is explicitly
supported by GCC.

Anyway, good to understand the rationale behind that 'volatile' stuff;
glad I wasn't missing something (too) tricky.

-John

On 10/1/20, Diego Nehab <[hidden email]> wrote:

> I think memcpy is the safest approach. Fortunately, a good compiler is
> smart enough to optimize away the memcpy when it knows the object is
> aligned. GCC even has a built-in to let you tell the compiler about the
> alignment when it is not obvious.
>
> On Thu, Oct 1, 2020 at 06:07 Andrew Gierth <[hidden email]>
> wrote:
>
>> >>>>> "Viacheslav" == Viacheslav Usov <[hidden email]> writes:
>>
>>
>>
>>  Viacheslav> Moreover, in C++ union-based type punning is undefined
>>
>>  Viacheslav> behavior, and since the Lua source code intends to be
>>
>>  Viacheslav> consumable as C++, this is a problem that volatile does not
>>
>>  Viacheslav> correctly address, either.
>>
>>
>>
>>  Viacheslav> I believe memcpy() is the only way that would be truly
>>
>>  Viacheslav> portable with all those things considered.
>>
>>
>>
>> I think you are correct, though I'd be surprised if any C++ compiler did
>>
>> the wrong thing with union type punning of basic types.
>>
>>
>>
>> The C++ standard seems to have the same exception for aliasing a value
>>
>> by way of a char or unsigned char type that C has. So your example has
>>
>> one more copy than is needed - the bytes can be copied directly from or
>>
>> to the "double" var without needing the intermediate char buff[].
>>
>>
>>
>> --
>>
>> Andrew.
>>
>>
>
> --
> Sent from my phone
>
Reply | Threaded
Open this post in threaded view
|

Re: Understanding 'volatile' in lstrlib.cpp (Lua 5.3)

Roberto Ierusalimschy
> Just another data point, since I recently saw it:
> The well-used GLM library (which is C++) uses type punning through a
> union just about as clear as can be:
>     https://github.com/g-truc/glm/blob/b3f87720261d623986f164b2a7f6a0a938430271/glm/detail/func_common.inl#L722
>
> I suppose, since it's C++ (not C90), it is technically undefined
> behavior. But I have certainly worked on dozens of systems over
> decades that rely on it, so it seems like extremely reliable undefined
> behavior, as far as that goes (:  And of course, it is explicitly
> supported by GCC.

I just learned that C99 was amended about that, to make clear that this
kind of tricks with unions behave as expected [1]. It now includes
the following footnote in 6.5.2.3p3:

    82) If the member used to access the contents of a union object
    is not the same as the member last used to store a value in the
    object, the appropriate part of the object representation of the
    value is reinterpreted as an object representation in the new type
    as described in 6.2.6 (a process sometimes called "type punning").
    This might be a trap representation.

(My version of the standard does not have these amendments.)

[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

-- Roberto