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

Mon Nov 9 13:30:34 PST 2015

----- Original Message -----

> From: "David Blaikie via llvm-commits" <llvm-commits at lists.llvm.org>
> To: "Xinliang David Li" <davidxl at google.com>
> Cc: "llvm-commits" <llvm-commits at lists.llvm.org>
> Sent: Monday, November 9, 2015 3:09:28 PM
> Subject: Re: [llvm] r252099 - Define portable macros for packed
> struct definitions:

> 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?
You can access objects of any type using char* (or unsigned char *) without running afoul of C/C++ TBAA rules. However, the underlying dynamic type of the object must actually be foo (or some cv-qualified version, an aggregate or union containing a foo, a derived class, etc.) to access it through a foo*. 

-Hal 

> 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
> > > > > 
> > > > 
> > > 
> > 
> 

> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

-- 

Hal Finkel 
Assistant Computational Scientist 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151109/b32b0ae7/attachment.html>