[llvm] r226490 - IR: Remove direct comparisons against Metadata::Storage, NFC

[Timur Iskhodzhanov]

This is what I get on my Windows machine:
  $ cat test.cpp
  char hello;
  $ bin\clang.exe -cc1 -gline-tables-only -triple i686-pc-linux -emit-obj
-o z test.cpp
  Assertion failed: DbgNode->isTemporary() && "Expected temporary node",
file ..\lib\IR\DebugInfo.cpp, line 350
but on Linux it's fine.

Replacing
  StorageType Storage : 2;
with
  unsigned Storage : 2;
seems to resolve the issue.
Shall I commit this change?

Reid might know the enum stuff better.

On Tue Jan 20 2015 at 6:39:22 PM Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2015 Jan 20, at 07:32, Timur Iskhodzhanov <timurrrr at google.com>
> wrote:
> >
> >
> >
> > On Tue Jan 20 2015 at 6:20:34 PM Duncan Exon Smith <dexonsmith at apple.com>
> wrote:
> >
> >
> > On Jan 20, 2015, at 3:48 AM, Timur Iskhodzhanov <timurrrr at google.com>
> wrote:
> >
> >> Hi Duncan,
> >>
> >> This has broken down "ninja check-asan" on 32-bit Windows.
> >> Assertion failed: Context.hasReplaceableUses() && "Expected RAUW
> support", file C:\src\llvm_bot\slave\win\build\llvm\include\llvm/IR/Metadata.h,
> line 976
> >
> > Can you include the asan output?   A backtrace?   Point at a buildbot?
>  I don't have the tools to reproduce this.
> >
> > There's no public buildbot for ASan itself, but turns out this problem
> is not even specific to ASan.
> >
> > This public bot just builds Chromium with Clang
> > http://build.chromium.org/p/chromium.fyi/builders/
> CrWinClang/builds/1295/steps/compile/logs/stdio
> >
> > Assertion failed: isTemporary() && "Expected temporary node", file
> C:\b\build\slave\CrWinClang\build\src\third_party\llvm\include\llvm/IR/Metadata.h,
> line 741
> > since r226499.
> >
> > OK, I'll try to minimize...
> > (I suppose the change looks sane at the first glance and you can't come
> up with a repro yourself)
> >
> > It seems to be hard to revert now...
> >> You've committed a lot of small changes in a very short timeframe -
> >
> > Actually, I committed work over the span of an entire work day.
> >
> > Oh, I see.
> >> - 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...
> >
> > 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.
> >
> > Unless it results in complex history and back-in-forth changes, I
> suppose.
> > 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.
>
> Yup, all the others depend on it.  I would have dealt with it myself
> if a buildbot had told me about it.
>
> > Do you think it's reasonable to temporarily revert your changes in the
> [r226490, HEAD] range to unbreak stuff?
> > I have a patch ready for that.
>
> Certainly an option, although I'm actively looking at this right now
> (sorry for the delay, I'm in PDT so I just woke up).
>
> Did you do a full bisect?  Are you sure that leaving this in and
> reverting the ones that follow don't also fix it?
>
> Do you know if MSVC handles things like:
>
>     enum StorageType { ... };
>     StorageType Storage : 2;
>
> correctly (well, the way I expect), or should I store it as an
>
>     unsigned Storage : 2;
>
> for compatibility?  Any chance it causes a weird layout problem that
> this commit happens to point out?
>
> (Given that this is MSVC-only, I suspect there's a layout problem or
> UB somewhere.)
>
> >> On Mon Jan 19 2015 at 10:30:36 PM Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> >> Author: dexonsmith
> >> Date: Mon Jan 19 13:26:24 2015
> >> New Revision: 226490
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=226490&view=rev
> >> Log:
> >> IR: Remove direct comparisons against Metadata::Storage, NFC
> >>
> >> Modified:
> >>     llvm/trunk/lib/IR/Metadata.cpp
> >>
> >> Modified: llvm/trunk/lib/IR/Metadata.cpp
> >> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/
> Metadata.cpp?rev=226490&r1=226489&r2=226490&view=diff
> >> ============================================================
> ==================
> >> --- llvm/trunk/lib/IR/Metadata.cpp (original)
> >> +++ llvm/trunk/lib/IR/Metadata.cpp Mon Jan 19 13:26:24 2015
> >> @@ -402,7 +402,7 @@ MDNode::MDNode(LLVMContext &Context, uns
> >>    for (unsigned I = 0, E = MDs.size(); I != E; ++I)
> >>      setOperand(I, MDs[I]);
> >>
> >> -  if (Storage == Temporary)
> >> +  if (isTemporary())
> >>      this->Context.makeReplaceable(
> >>          make_unique<ReplaceableMetadataImpl>(Context));
> >>  }
> >> @@ -416,7 +416,7 @@ static bool isOperandUnresolved(Metadata
> >>  UniquableMDNode::UniquableMDNode(LLVMContext &C, unsigned ID,
> >>                                   StorageType Storage,
> ArrayRef<Metadata *> Vals)
> >>      : MDNode(C, ID, Storage, Vals) {
> >> -  if (Storage != Uniqued)
> >> +  if (!isUniqued())
> >>      return;
> >>
> >>    // Check whether any operands are unresolved, requiring re-uniquing.
> >> @@ -432,7 +432,7 @@ UniquableMDNode::UniquableMDNode(LLVMCon
> >>  }
> >>
> >>  void UniquableMDNode::resolve() {
> >> -  assert(Storage == Uniqued && "Expected this to be uniqued");
> >> +  assert(isUniqued() && "Expected this to be uniqued");
> >>    assert(!isResolved() && "Expected this to be unresolved");
> >>
> >>    // Move the map, so that this immediately looks resolved.
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150120/720233f0/attachment.html>