[llvm] r289017 - Unbreak buildbots where the debug info test was crashing due to unchecked error.

Mon Dec 12 15:43:38 PST 2016

On Mon, Dec 12, 2016 at 3:37 PM Greg Clayton <gclayton at apple.com> wrote:

> The other thing is I don't want this to actually fail if the target isn't
> available (the most common error).

Ah, I assumed the existing code:

  ::testing::AssertionFailure() << "error: " << ErrorMsg;

Caused a failure in this case. Does it not?

> If you build the "ARM" targets only on x86 system, it will try to get the
> host architecture's 32 and 64 bit variants and either can fail, but we
> don't want this breaking the buildbots by failing a test. We only need a
> target to test 4 and 8 byte addresses and also because we need the whole
> target stack just to use the AsmPrinter so we can generate the DWARF. I am
> open to better ideas on how to fix this test to do something better. But we
> do want the majority of our buildbots to test the 4 and 8 byte addresses as
> much as possible. Right now if the target fails to be found, we just return
> early from the DWARF tests and don't create an error. The only other idea I
> have is to iterate through all targets and run all targets through the
> DWARF generator tests, but I didn't as that is probably not very
> inefficient use of testing time.
>
> Greg
>
>
> > On Dec 12, 2016, at 2:54 PM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> > Oh, sorry - I see, you meant the macro that enables checking in
> llvm::Error, right.
> >
> > The thing is it doesn't respect your checking an Expected<T>, just
> checking the llvm::Error.
> >
> > So something like this. Actually, I think you don't even need the
> boolean return:
> >
> >   handleAllErrors(Expecte.takeError(), [](const llvm::ErrorInfoBase &EI)
> {
> >     FAIL() << EI.message();
> >   });
> >
> > This should produce a fatal failure, which terminates gtest execution
> (with an exception or otherwise), so the code after this shouldn't need to
> conditionally return or anything - the fact that this function returns at
> all is sufficient. I can double check this for you if you like - but pretty
> sure that's how the gtest documentation reads. (FAIL is like
> ASSERT_TRUE(false) - assertions are fatal checks, terminating execution of
> the test)
> >
> >
> https://github.com/google/googletest/blob/master/googletest/docs/Primer.md
> >
> https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md
> >
> > (at this point, maybe the 3 lines are short enough you could put them
> inline ratehr than having a separate function - but maybe not? Up to you)
> >
> >
> > On Mon, Dec 12, 2016 at 2:38 PM Greg Clayton <gclayton at apple.com> wrote:
> >
> > > On Dec 12, 2016, at 2:30 PM, David Blaikie <dblaikie at gmail.com> wrote:
> > >
> > >
> > >
> > > On Mon, Dec 12, 2016 at 2:05 PM Greg Clayton <gclayton at apple.com>
> wrote:
> > >
> > > > On Dec 12, 2016, at 1:59 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> > > >
> > > >
> > > >
> > > > On Wed, Dec 7, 2016 at 6:21 PM Greg Clayton via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> > > > Author: gclayton
> > > > Date: Wed Dec  7 20:11:03 2016
> > > > New Revision: 289017
> > > >
> > > > URL: http://llvm.org/viewvc/llvm-project?rev=289017&view=rev
> > > > Log:
> > > > Unbreak buildbots where the debug info test was crashing due to
> unchecked error.
> > > >
> > > >
> > > > Modified:
> > > >     llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
> > > >
> > > > Modified: llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
> > > > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp?rev=289017&r1=289016&r2=289017&view=diff
> > > >
> ==============================================================================
> > > > --- llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
> (original)
> > > > +++ llvm/trunk/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp Wed
> Dec  7 20:11:03 2016
> > > > @@ -51,14 +51,15 @@ Triple getHostTripleForAddrSize(uint8_t
> > > >  /// \returns true if there were errors, false otherwise.
> > > >  template <typename T>
> > > >  static bool HandleExpectedError(T &Expected) {
> > > > -  if (!Expected)
> > > > -    return false;
> > > >    std::string ErrorMsg;
> > > >    handleAllErrors(Expected.takeError(), [&](const
> llvm::ErrorInfoBase &EI) {
> > > >      ErrorMsg = EI.message();
> > > >    });
> > > > -  ::testing::AssertionFailure() << "error: " << ErrorMsg;
> > > > -  return true;
> > > > +  if (!ErrorMsg.empty()) {
> > > > +    ::testing::AssertionFailure() << "error: " << ErrorMsg;
> > > > +    return true;
> > > > +  }
> > > > +  return false;
> > > >
> > > > I think this could be written a bit simpler and without relying on
> the error string being non-empty, etc.
> > > >
> > > > Oh, huh - Lang, any ideas?
> > > >
> > > > It'd be nice if handleAllErrors could return a value (if all
> handlers returned the same/compatibible) - though I guess then it'd need a
> default value.
> > > >
> > > > Lang, would it be more idiomatic here to test the error for
> non-failure ,return false, otherwise then handleAllErrors and return true.
> > > >
> > >
> > > As you can see I was testing the error and returning false, but that
> was crashing some buildbots with some extra API macro defined.
> > >
> > > Got a link/quote of a buildbot failure - that sounds like a rather
> different problem from what this code would address, so I'm probably not
> picturing what sort of failure you're describing.
> >
> > without the extra API define, the initial code works fine. With the
> extra API define, we get an error stating no one checked the error.
> >
> > >
> > > I have been looking for Lang around here but he is on vacation. As
> soon as he gets back I will check with him and make this work, but for now
> I didn't want buildbots to be broken.
> > >
> > > Greg
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161212/8a48c687/attachment.html>