[llvm] r226490 - IR: Remove direct comparisons against Metadata::Storage, NFC
Duncan P. N. Exon Smith
dexonsmith at apple.com
Tue Jan 20 09:07:24 PST 2015
> 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.
>
> > 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
> >
>
More information about the llvm-commits
mailing list