<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 9, 2015 at 1:33 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Mon, Nov 9, 2015 at 1:09 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>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:0 0 0 .8ex;border-left:1px #ccc solid;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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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:0 0 0 .8ex;border-left:1px #ccc solid;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></span><div>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.<br></div></div></div></div></blockquote><div><br></div></span><div>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). </div></div></div></div></blockquote><div><br></div><div>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).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> 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.</div></div></div></div></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><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></div></div></div></div></blockquote><div><br></div></span><div>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. </div></div></div></div></blockquote><div><br></div><div>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.<br><br>- David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="HOEnZb"><font color="#888888"><div><br></div><div>David</div></font></span><div><div class="h5"><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>- Dave</div><div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>