[llvm] r252099 - Define portable macros for packed struct definitions:

[David Blaikie via llvm-commits]

On Mon, Nov 9, 2015 at 1:33 PM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> On Mon, Nov 9, 2015 at 1:09 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Thu, Nov 5, 2015 at 9:11 AM, Xinliang David Li <davidxl at google.com>
>> wrote:
>>
>>>
>>>
>>> On Thu, Nov 5, 2015 at 12:02 AM, David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Wed, Nov 4, 2015 at 9:36 PM, Xinliang David Li <davidxl at google.com>
>>>> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Wed, Nov 4, 2015 at 8:18 PM, David Blaikie <dblaikie at gmail.com>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Nov 4, 2015 at 3:42 PM, Xinliang David Li via llvm-commits <
>>>>>> llvm-commits at lists.llvm.org> wrote:
>>>>>>
>>>>>>> Author: davidxl
>>>>>>> Date: Wed Nov  4 17:42:56 2015
>>>>>>> New Revision: 252099
>>>>>>>
>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=252099&view=rev
>>>>>>> Log:
>>>>>>> Define portable macros for packed struct definitions:
>>>>>>>
>>>>>>>  1. A macro with argument: LLVM_PACKED(StructDefinition)
>>>>>>>  2. A pair of macros defining scope of region with packing:
>>>>>>>     LLVM_PACKED_START
>>>>>>>      struct A { ... };
>>>>>>>      struct B { ... };
>>>>>>>     LLVM_PACKED_END
>>>>>>>
>>>>>>
>>>>>> What are we planning to use these for (I imagine there are already
>>>>>> some uses of packed macros/etc in various parts of LLVM that this will be
>>>>>> used to replace? What are they? What new uses do you have in mind?) I'm a
>>>>>> little concerned, because this tend to leads to non-portable code trying to
>>>>>> parse & then use structs with misaligned members, etc... (which, if I
>>>>>> understand correctly, is only a perf penalty on X86, but potentially a
>>>>>> correctness problem on other platforms we support/care about (not to
>>>>>> mention the known bits stuff we do might assume certain bits are zero when
>>>>>> they aren't, etc))
>>>>>>
>>>>>>
>>>>>
>>>>> This is used for existing runtime data structure (Coverage Map's
>>>>> Function Record). Previously the structure is not explicitly declared but
>>>>> referenced with manually specified layout and alignment.
>>>>>
>>>>
>>>> Not sure I quite follow this ^ sorry - could you give an example?
>>>> (perhaps this commit could've included an example of porting over an
>>>> existing use to use the new macros from whatever the code was doing
>>>> previously)
>>>>
>>>>
>>>
>>>
>> See http://reviews.llvm.org/D13843 and see my replies there.
>>>
>>
>> That code still looks solidly like UB to me - it's an aliasing violation
>> to treat the char* as a foo*, is it not? I think the original code was
>> better. You can replace the size and offset calculations with
>> sizeof+offsetof, but the pointer aliasing and actually using the packed
>> struct seem incorrect.
>>
>
> The underlying type of the data is the packed struct which is intended (if
> you look at the instrumentation code that generates the type in the first
> place in tools/clang/CodeGen/lib/CoverageMappingGen.cpp).
>

Is that within the same process? If it's written to disk and read back in
to a foreign process that doesn't hold anymore (foreign process could be on
a different architecture, etc, etc).


> Having char* type pointer point to the struct does not violate any
> aliasing rule.  Otherwise, files with structured format can not be accessed
> using structs any more.
>

That's the thing - my understanding is that they can't. Struct mapping over
a file or network protocol buffer is UB, though commonly used technique, so
far as I understand.


>
>
>>
>> I don't know if this code is at least portable to all the compilers we
>> support (eg: the guarantees about unaligned loads of members of the struct,
>> etc), but even then, I'd avoid writing code using such extensions when
>> there's safe and more portable alternatives that don't seem too costly, no?
>>
>
> The code uses the type of the underlying object to access the object, and
> as I said it is legacy -- it can be designed in a different way to avoid
> it.
>

The original code that was replaced in that review did not have the UB
problem, though - it just accessed the members as byte offsets and used
well defined byte shifting to parse the values, rather than reinterpreting
the raw buffer as a struct (or int, etc) to read the values.

- David


>
> David
>
>
>
>>
>> - Dave
>>
>>
>>>
>>>
>>>
>>>> New uses of the macro should be done with care -- but there might be
>>>>> legit reason to use it though, let's not rule that out.
>>>>>
>>>>
>>>> Might be, though ideally I'd prefer to see at least a motivating use
>>>> case before adding tools like this, but I'm not strongly invested/pushing
>>>> back here.
>>>>
>>>
>>> I have plans to restructure the profile data structures. If I manage to
>>> get rid of the support of the legacy format, we can revisit this topic
>>> (about the pros and cons etc).
>>>
>>> David
>>>
>>>
>>>>
>>>>
>>>>>
>>>>> David
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>> Differential Revision: http://reviews.llvm.org/D14337
>>>>>>>
>>>>>>> Modified:
>>>>>>>     llvm/trunk/include/llvm/Support/Compiler.h
>>>>>>>
>>>>>>> Modified: llvm/trunk/include/llvm/Support/Compiler.h
>>>>>>> URL:
>>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Compiler.h?rev=252099&r1=252098&r2=252099&view=diff
>>>>>>>
>>>>>>> ==============================================================================
>>>>>>> --- llvm/trunk/include/llvm/Support/Compiler.h (original)
>>>>>>> +++ llvm/trunk/include/llvm/Support/Compiler.h Wed Nov  4 17:42:56
>>>>>>> 2015
>>>>>>> @@ -293,6 +293,34 @@
>>>>>>>  # define LLVM_ALIGNAS(x) alignas(x)
>>>>>>>  #endif
>>>>>>>
>>>>>>> +/// \macro LLVM_PACKED
>>>>>>> +/// \brief Used to specify a packed structure.
>>>>>>> +/// LLVM_PACKED(
>>>>>>> +///    struct A {
>>>>>>> +///      int i;
>>>>>>> +///      int j;
>>>>>>> +///      int k;
>>>>>>> +///      long long l;
>>>>>>> +///   });
>>>>>>> +///
>>>>>>> +/// LLVM_PACKED_START
>>>>>>> +/// struct B {
>>>>>>> +///   int i;
>>>>>>> +///   int j;
>>>>>>> +///   int k;
>>>>>>> +///   long long l;
>>>>>>> +/// };
>>>>>>> +/// LLVM_PACKED_END
>>>>>>> +#ifdef _MSC_VER
>>>>>>> +# define LLVM_PACKED(d) __pragma(pack(push, 1)) d
>>>>>>> __pragma(pack(pop))
>>>>>>> +# define LLVM_PACKED_START __pragma(pack(push, 1))
>>>>>>> +# define LLVM_PACKED_END   __pragma(pack(pop))
>>>>>>> +#else
>>>>>>> +# define LLVM_PACKED(d) d __attribute__((packed))
>>>>>>> +# define LLVM_PACKED_START _Pragma("pack(push, 1)")
>>>>>>> +# define LLVM_PACKED_END   _Pragma("pack(pop)")
>>>>>>> +#endif
>>>>>>> +
>>>>>>>  /// \macro LLVM_PTR_SIZE
>>>>>>>  /// \brief A constant integer equivalent to the value of
>>>>>>> sizeof(void*).
>>>>>>>  /// Generally used in combination with LLVM_ALIGNAS or when doing
>>>>>>> computation in
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> llvm-commits mailing list
>>>>>>> llvm-commits at lists.llvm.org
>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151109/307a4db7/attachment.html>