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

David Blaikie dblaikie at gmail.com
Tue Jan 20 10:14:58 PST 2015 

On Tue, Jan 20, 2015 at 9:07 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2015 Jan 20, at 08:38, Timur Iskhodzhanov <timurrrr at google.com>
> wrote:
> >
> > On Tue Jan 20 2015 at 6:58:52 PM Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > > On 2015 Jan 20, at 07:53, Timur Iskhodzhanov <timurrrr at google.com>
> wrote:
> > >
> > > 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?
> > >
> >
> > Thanks for tracking this down!
> >
> > You've just won a black belt in remote debugging :)
> >
> > I guessed at the same thing and got someone on IRC to confirm.  Already
> > committed in r226570.
> >
> > Thanks, the Chromium bots are cycling green now!
> >
> > > Reid might know the enum stuff better.
> >
> > It's not an enum problem, as it turns out.  Same difference in behaviour
> > would happen with `int Storage : 2`.
> >
> > MSVC makes the top bit of the bitfield the sign bit; other compilers
> > "chop off" the sign bit.  It's implementation-defined which behaviour
> > you get (so MSVC isn't actually wrong here, just annoyingly different).
> >
> > I should have remembered this :/.  Sorry for the trouble.
> >
> > Do you think we could somehow find such bugs automatically?
>
> If you had a bot running with your configuration sending emails and/or
> yelling at the IRC channel, I would have noticed pretty soon after my
> commit :).
>
> > And/or update the developer policy?
>
> Sure.  I think this sort of thing belongs in the coding standards [1],
> although I'm not quite sure where to put it.
>
> [1]: http://llvm.org/docs/CodingStandards.html
>
> Feel free to submit a patch!
>
> BTW, Aaron Ballman (on IRC) pointed out that there's specific language in
> the standard to deal with enum bitfields:
>
> >  [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. "
>
> So this probably *is* an MSVC bug.  Not that it matters too much; we need
> to avoid it either way.
>

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.


>
>
> >
> > > 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
> > >
> >
>
>
> _______________________________________________
> 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/abd35761/attachment.html>

More information about the llvm-commits mailing list