<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Sep 15, 2014 at 3:51 PM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> writes:<br>
> On Fri, Sep 12, 2014 at 2:22 PM, Justin Bogner <<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a>> wrote:<br>
><br>
> Author: bogner<br>
> Date: Fri Sep 12 16:22:55 2014<br>
> New Revision: 217708<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=217708&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=217708&view=rev</a><br>
> Log:<br>
> llvm-profdata: Avoid undefined behaviour when reading raw profiles<br>
><br>
> The raw profiles that are generated in compiler-rt always add padding<br>
> so that each profile is aligned, so we can simply treat files that<br>
> don't have this property as malformed.<br>
><br>
> Any chance of changing this to not rely on UB (this code looks like it's<br>
> violating C++ aliasing rules pretty consistently) and instead using memcpy?<br>
<br>
</span>It's always casting from char *, which IIUC doesn't break aliasing<br>
rules, so I *think* that this isn't relying on UB.<br></blockquote><div><br>[+Richard for some language lawyering]<br><br></div><div>I believe the rules are:<br><br>You can take a pointer to any entity, cast it to a char* then back, and continue using it as the original type. But this only works if the pointer was originally a pointer to that type you're dereferencing by, which isn't the case here.<br><br>You can take a pointer to T (where T is a standard layout(?)), cast to pointer to char, read the bytes of the object then write them back into some other object (via a char* that actually points to a T - derived the same way as the original) you can do that safely.<br><br>But none of this allows taking a char* that wasn't derived from a T*, casting it to a T* and reading.<br><br>But this is just my high-level understanding, I don't actually know where the words are in the standard to support this position. Hoping Richard can fill us in/confirm/deny and/or I can go hunting.<br><br>- David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Assuming I'm right about that, the current design avoids unnecessarily<br>
copying from the file to an auxiliary structure just to read the data<br>
and throw it away.<br>
<div class="HOEnZb"><div class="h5"><br>
> Caught by Alexey's new ubsan bot. Thanks!<br>
><br>
> Modified:<br>
> llvm/trunk/lib/ProfileData/InstrProfReader.cpp<br>
> llvm/trunk/test/tools/llvm-profdata/raw-two-profiles.test<br>
><br>
> Modified: llvm/trunk/lib/ProfileData/InstrProfReader.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/</a><br>
> InstrProfReader.cpp?rev=217708&r1=217707&r2=217708&view=diff<br>
> ==========================================================================<br>
> ====<br>
> --- llvm/trunk/lib/ProfileData/InstrProfReader.cpp (original)<br>
> +++ llvm/trunk/lib/ProfileData/InstrProfReader.cpp Fri Sep 12 16:22:55<br>
> 2014<br>
> @@ -190,6 +190,9 @@ RawInstrProfReader<IntPtrT>::readNextHea<br>
> // garbage at the end of the file.<br>
> if (CurrentPos + sizeof(RawHeader) > End)<br>
> return instrprof_error::malformed;<br>
> + // The writer ensures each profile is padded to start at an aligned<br>
> address.<br>
> + if (reinterpret_cast<size_t>(CurrentPos) % alignOf<uint64_t>())<br>
> + return instrprof_error::malformed;<br>
> // The magic should have the same byte order as in the previous header.<br>
> uint64_t Magic = *reinterpret_cast<const uint64_t *>(CurrentPos);<br>
> if (Magic != swap(getRawMagic<IntPtrT>()))<br>
><br>
> Modified: llvm/trunk/test/tools/llvm-profdata/raw-two-profiles.test<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/</a><br>
> llvm-profdata/raw-two-profiles.test?rev=217708&r1=217707&r2=217708&view=<br>
> diff<br>
> ==========================================================================<br>
> ====<br>
> --- llvm/trunk/test/tools/llvm-profdata/raw-two-profiles.test (original)<br>
> +++ llvm/trunk/test/tools/llvm-profdata/raw-two-profiles.test Fri Sep 12<br>
> 16:22:55 2014<br>
> @@ -39,11 +39,9 @@ RUN: printf '\0\0\0\0\0' >> %t-foo-padde<br>
> RUN: cat %t-bar.profraw > %t-bar-padded.profraw<br>
> RUN: printf '\0\0\0\0\0' >> %t-bar-padded.profraw<br>
><br>
> -RUN: cat %t-foo.profraw %t-bar.profraw > %t-nopad.profraw<br>
> RUN: cat %t-foo-padded.profraw %t-bar.profraw > %t-pad-between.profraw<br>
> RUN: cat %t-foo-padded.profraw %t-bar-padded.profraw > %t-pad.profraw<br>
><br>
> -RUN: llvm-profdata show %t-nopad.profraw -all-functions -counts |<br>
> FileCheck %s<br>
> RUN: llvm-profdata show %t-pad-between.profraw -all-functions -counts |<br>
> FileCheck %s<br>
> RUN: llvm-profdata show %t-pad.profraw -all-functions -counts | FileCheck<br>
> %s<br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>