<html><head><style type='text/css'>p { margin: 0; }</style></head><body><div style='font-family: arial,helvetica,sans-serif; font-size: 10pt; color: #000000'><br><hr id="zwchr"><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><b>From: </b>"David Blaikie via llvm-commits" <llvm-commits@lists.llvm.org><br><b>To: </b>"Xinliang David Li" <davidxl@google.com><br><b>Cc: </b>"llvm-commits" <llvm-commits@lists.llvm.org><br><b>Sent: </b>Monday, November 9, 2015 3:09:28 PM<br><b>Subject: </b>Re: [llvm] r252099 - Define portable macros for packed struct definitions:<br><br><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 5, 2015 at 9:11 AM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Thu, Nov 5, 2015 at 12:02 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Wed, Nov 4, 2015 at 9:36 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Wed, Nov 4, 2015 at 8:18 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 4, 2015 at 3:42 PM, Xinliang David Li via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">Author: davidxl<br>
Date: Wed Nov 4 17:42:56 2015<br>
New Revision: 252099<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=252099&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=252099&view=rev</a><br>
Log:<br>
Define portable macros for packed struct definitions:<br>
<br>
1. A macro with argument: LLVM_PACKED(StructDefinition)<br>
2. A pair of macros defining scope of region with packing:<br>
LLVM_PACKED_START<br>
struct A { ... };<br>
struct B { ... };<br>
LLVM_PACKED_END<br></blockquote><div><br></div><div>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))</div><div> </div></div></div></div></blockquote><div><br></div></span><div>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.</div></div></div></div></blockquote><div><br></div></span><div>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)</div><span><div> </div></span></div></div></div></blockquote><div> <br></div></span></div></div></div></blockquote><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div></div></span><div>See <a href="http://reviews.llvm.org/D13843" target="_blank">http://reviews.llvm.org/D13843</a> and see my replies there.</div></div></div></div></blockquote><div><br></div><div id="DWT57653">That code still looks solidly like UB to me - it's an aliasing violation to treat the char* as a foo*, is it not?</div></div></div></div></blockquote><br>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*.<br><br> -Hal<br><br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> 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.<br><br>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?<br><br>- Dave</div><div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div><br></div><div> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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.<br></div></div></div></div></blockquote><div><br></div></span><div>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.</div><div><div><div></div></div></div></div></div></div></blockquote><div><br></div></span><div>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). </div><span><font color="#888888"><div><br></div><div>David</div></font></span><div><div><div> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><div> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><font color="#888888"><div><br></div><div>David</div></font></span><div><div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
Differential Revision: <a href="http://reviews.llvm.org/D14337" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14337</a><br>
<br>
Modified:<br>
llvm/trunk/include/llvm/Support/Compiler.h<br>
<br>
Modified: llvm/trunk/include/llvm/Support/Compiler.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Compiler.h?rev=252099&r1=252098&r2=252099&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Compiler.h?rev=252099&r1=252098&r2=252099&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Support/Compiler.h (original)<br>
+++ llvm/trunk/include/llvm/Support/Compiler.h Wed Nov 4 17:42:56 2015<br>
@@ -293,6 +293,34 @@<br>
# define LLVM_ALIGNAS(x) alignas(x)<br>
#endif<br>
<br>
+/// \macro LLVM_PACKED<br>
+/// \brief Used to specify a packed structure.<br>
+/// LLVM_PACKED(<br>
+/// struct A {<br>
+/// int i;<br>
+/// int j;<br>
+/// int k;<br>
+/// long long l;<br>
+/// });<br>
+///<br>
+/// LLVM_PACKED_START<br>
+/// struct B {<br>
+/// int i;<br>
+/// int j;<br>
+/// int k;<br>
+/// long long l;<br>
+/// };<br>
+/// LLVM_PACKED_END<br>
+#ifdef _MSC_VER<br>
+# define LLVM_PACKED(d) __pragma(pack(push, 1)) d __pragma(pack(pop))<br>
+# define LLVM_PACKED_START __pragma(pack(push, 1))<br>
+# define LLVM_PACKED_END __pragma(pack(pop))<br>
+#else<br>
+# define LLVM_PACKED(d) d __attribute__((packed))<br>
+# define LLVM_PACKED_START _Pragma("pack(push, 1)")<br>
+# define LLVM_PACKED_END _Pragma("pack(pop)")<br>
+#endif<br>
+<br>
/// \macro LLVM_PTR_SIZE<br>
/// \brief A constant integer equivalent to the value of sizeof(void*).<br>
/// Generally used in combination with LLVM_ALIGNAS or when doing computation in<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>
<br>_______________________________________________<br>llvm-commits mailing list<br>llvm-commits@lists.llvm.org<br>http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits<br></blockquote><br><br><br>-- <br><div><span name="x"></span>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<span name="x"></span><br></div></div></body></html>