<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 20, 2015 at 9:07 AM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
> On 2015 Jan 20, at 08:38, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>> wrote:<br>
><br>
> On Tue Jan 20 2015 at 6:58:52 PM Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> > On 2015 Jan 20, at 07:53, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>> wrote:<br>
> ><br>
> > This is what I get on my Windows machine:<br>
> > $ cat test.cpp<br>
> > char hello;<br>
> > $ bin\clang.exe -cc1 -gline-tables-only -triple i686-pc-linux -emit-obj -o z test.cpp<br>
> > Assertion failed: DbgNode->isTemporary() && "Expected temporary node", file ..\lib\IR\DebugInfo.cpp, line 350<br>
> > but on Linux it's fine.<br>
> ><br>
> > Replacing<br>
> > StorageType Storage : 2;<br>
> > with<br>
> > unsigned Storage : 2;<br>
> > seems to resolve the issue.<br>
> > Shall I commit this change?<br>
> ><br>
><br>
> Thanks for tracking this down!<br>
><br>
> You've just won a black belt in remote debugging :)<br>
><br>
> I guessed at the same thing and got someone on IRC to confirm. Already<br>
> committed in r226570.<br>
><br>
> Thanks, the Chromium bots are cycling green now!<br>
><br>
> > Reid might know the enum stuff better.<br>
><br>
> It's not an enum problem, as it turns out. Same difference in behaviour<br>
> would happen with `int Storage : 2`.<br>
><br>
> MSVC makes the top bit of the bitfield the sign bit; other compilers<br>
> "chop off" the sign bit. It's implementation-defined which behaviour<br>
> you get (so MSVC isn't actually wrong here, just annoyingly different).<br>
><br>
> I should have remembered this :/. Sorry for the trouble.<br>
><br>
> Do you think we could somehow find such bugs automatically?<br>
<br>
</div></div>If you had a bot running with your configuration sending emails and/or<br>
yelling at the IRC channel, I would have noticed pretty soon after my<br>
commit :).<br>
<span class=""><br>
> And/or update the developer policy?<br>
<br>
</span>Sure. I think this sort of thing belongs in the coding standards [1],<br>
although I'm not quite sure where to put it.<br>
<br>
[1]: <a href="http://llvm.org/docs/CodingStandards.html" target="_blank">http://llvm.org/docs/CodingStandards.html</a><br>
<br>
Feel free to submit a patch!<br>
<br>
BTW, Aaron Ballman (on IRC) pointed out that there's specific language in<br>
the standard to deal with enum bitfields:<br>
<br>
> [class.bit] calls out enumerations explicitly: "If the value of an enumerator is stored into a bit-field of the same enumeration type and the number of bits in the bit-field is large enough to hold all the values of that enumeration type (7.2), the original enumerator value and the value of the bit-field shall compare equal. "<br>
<br>
So this probably *is* an MSVC bug. Not that it matters too much; we need<br>
to avoid it either way.<br></blockquote><div><br>Yeah, it's one of those bits of community lore that one should steer clear of enum bitfields because of MSVC and signed bitfields. I never remember the specifics of why they're bad, just that they are - agreed, it can/should be written down somewhere (people will still forget, but at least we'll have the explanation written somewhere, etc). Maybe the style guide needs an expanded bit of "things not to do" which would include the existing bit of which C++11 features we can use (links to compatibility matrices for supported compilers, maybe highlight common gotchas like MSVC's lack of compiler-generated move ops) this enum thing, etc.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
<br>
><br>
> > On Tue Jan 20 2015 at 6:39:22 PM Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> ><br>
> > > On 2015 Jan 20, at 07:32, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>> wrote:<br>
> > ><br>
> > ><br>
> > ><br>
> > > On Tue Jan 20 2015 at 6:20:34 PM Duncan Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> > ><br>
> > ><br>
> > > On Jan 20, 2015, at 3:48 AM, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>> wrote:<br>
> > ><br>
> > >> Hi Duncan,<br>
> > >><br>
> > >> This has broken down "ninja check-asan" on 32-bit Windows.<br>
> > >> Assertion failed: Context.hasReplaceableUses() && "Expected RAUW support", file C:\src\llvm_bot\slave\win\build\llvm\include\llvm/IR/Metadata.h, line 976<br>
> > ><br>
> > > Can you include the asan output? A backtrace? Point at a buildbot? I don't have the tools to reproduce this.<br>
> > ><br>
> > > There's no public buildbot for ASan itself, but turns out this problem is not even specific to ASan.<br>
> > ><br>
> > > This public bot just builds Chromium with Clang<br>
> > > <a href="http://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/1295/steps/compile/logs/stdio" target="_blank">http://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/1295/steps/compile/logs/stdio</a><br>
> > ><br>
> > > Assertion failed: isTemporary() && "Expected temporary node", file C:\b\build\slave\CrWinClang\build\src\third_party\llvm\include\llvm/IR/Metadata.h, line 741<br>
> > > since r226499.<br>
> > ><br>
> > > OK, I'll try to minimize...<br>
> > > (I suppose the change looks sane at the first glance and you can't come up with a repro yourself)<br>
> > ><br>
> > > It seems to be hard to revert now...<br>
> > >> You've committed a lot of small changes in a very short timeframe -<br>
> > ><br>
> > > Actually, I committed work over the span of an entire work day.<br>
> > ><br>
> > > Oh, I see.<br>
> > >> - I assume you're using git? Is there a reason why you don't squash your changes before committing? It makes finding bad commits / reverting harder...<br>
> > ><br>
> > > Besides being developer policy to separate changes as much as possible, it also makes changes easier to review, and isolating which change causes a problem easier to determine.<br>
> > ><br>
> > > Unless it results in complex history and back-in-forth changes, I suppose.<br>
> > > I've spent more than an hour today trying to locally bisect and revert the changes and ended up just locally reverting 40+ of your commits after 226490 to make progress. Unfortunately, reverting just r226490 results in a waterfall of merge conflicts.<br>
> ><br>
> > Yup, all the others depend on it. I would have dealt with it myself<br>
> > if a buildbot had told me about it.<br>
> ><br>
> > > Do you think it's reasonable to temporarily revert your changes in the [r226490, HEAD] range to unbreak stuff?<br>
> > > I have a patch ready for that.<br>
> ><br>
> > Certainly an option, although I'm actively looking at this right now<br>
> > (sorry for the delay, I'm in PDT so I just woke up).<br>
> ><br>
> > Did you do a full bisect? Are you sure that leaving this in and<br>
> > reverting the ones that follow don't also fix it?<br>
> ><br>
> > Do you know if MSVC handles things like:<br>
> ><br>
> > enum StorageType { ... };<br>
> > StorageType Storage : 2;<br>
> ><br>
> > correctly (well, the way I expect), or should I store it as an<br>
> ><br>
> > unsigned Storage : 2;<br>
> ><br>
> > for compatibility? Any chance it causes a weird layout problem that<br>
> > this commit happens to point out?<br>
> ><br>
> > (Given that this is MSVC-only, I suspect there's a layout problem or<br>
> > UB somewhere.)<br>
> ><br>
> > >> On Mon Jan 19 2015 at 10:30:36 PM Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> > >> Author: dexonsmith<br>
> > >> Date: Mon Jan 19 13:26:24 2015<br>
> > >> New Revision: 226490<br>
> > >><br>
> > >> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=226490&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=226490&view=rev</a><br>
> > >> Log:<br>
> > >> IR: Remove direct comparisons against Metadata::Storage, NFC<br>
> > >><br>
> > >> Modified:<br>
> > >> llvm/trunk/lib/IR/Metadata.cpp<br>
> > >><br>
> > >> Modified: llvm/trunk/lib/IR/Metadata.cpp<br>
> > >> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Metadata.cpp?rev=226490&r1=226489&r2=226490&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Metadata.cpp?rev=226490&r1=226489&r2=226490&view=diff</a><br>
> > >> ==============================================================================<br>
> > >> --- llvm/trunk/lib/IR/Metadata.cpp (original)<br>
> > >> +++ llvm/trunk/lib/IR/Metadata.cpp Mon Jan 19 13:26:24 2015<br>
> > >> @@ -402,7 +402,7 @@ MDNode::MDNode(LLVMContext &Context, uns<br>
> > >> for (unsigned I = 0, E = MDs.size(); I != E; ++I)<br>
> > >> setOperand(I, MDs[I]);<br>
> > >><br>
> > >> - if (Storage == Temporary)<br>
> > >> + if (isTemporary())<br>
> > >> this->Context.makeReplaceable(<br>
> > >> make_unique<ReplaceableMetadataImpl>(Context));<br>
> > >> }<br>
> > >> @@ -416,7 +416,7 @@ static bool isOperandUnresolved(Metadata<br>
> > >> UniquableMDNode::UniquableMDNode(LLVMContext &C, unsigned ID,<br>
> > >> StorageType Storage, ArrayRef<Metadata *> Vals)<br>
> > >> : MDNode(C, ID, Storage, Vals) {<br>
> > >> - if (Storage != Uniqued)<br>
> > >> + if (!isUniqued())<br>
> > >> return;<br>
> > >><br>
> > >> // Check whether any operands are unresolved, requiring re-uniquing.<br>
> > >> @@ -432,7 +432,7 @@ UniquableMDNode::UniquableMDNode(LLVMCon<br>
> > >> }<br>
> > >><br>
> > >> void UniquableMDNode::resolve() {<br>
> > >> - assert(Storage == Uniqued && "Expected this to be uniqued");<br>
> > >> + assert(isUniqued() && "Expected this to be uniqued");<br>
> > >> assert(!isResolved() && "Expected this to be unresolved");<br>
> > >><br>
> > >> // Move the map, so that this immediately looks resolved.<br>
> > >><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>
> ><br>
><br>
<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>