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

[David Blaikie via llvm-commits]

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.

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?

- 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/dd85ecd9/attachment.html>