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

Mon Jan 9 14:58:16 PST 2017

Lang was out for a few weeks vacation and is now back. I will reproduce this and ask him about it.

Greg

> On Jan 9, 2017, at 8:22 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> Ping
> 
> On Tue, Dec 13, 2016 at 10:26 AM Greg Clayton <gclayton at apple.com <mailto:gclayton at apple.com>> wrote:
> 
> > On Dec 13, 2016, at 9:56 AM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
> >
> >
> >
> > On Tue, Dec 13, 2016 at 8:58 AM Greg Clayton <gclayton at apple.com <mailto:gclayton at apple.com>> wrote:
> >
> > > On Dec 12, 2016, at 3:43 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
> > >
> > >
> > >
> > > On Mon, Dec 12, 2016 at 3:37 PM Greg Clayton <gclayton at apple.com <mailto: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?
> >
> > Yes, you would think it would, but it doesn't...
> >
> > It also doesn't seem to print any output (it may print output on failure? But you don't want this case to fail - so that seems like it won't ever happen). Perhaps it's better to just ignore the error entirely (& exit the test, as you are doing)?
> >
> 
> Yeah if I can't get it to act reliably, I will fix it as mentioned above. I will speak with Lang when he gets back and figure out the correct usage of Expected and fix accordingly.
> 
> Greg
> 
> 
> > > 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 <mailto: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/Primer.md>
> > > > https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.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 <mailto:gclayton at apple.com>> wrote:
> > > >
> > > > > On Dec 12, 2016, at 2:30 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On Mon, Dec 12, 2016 at 2:05 PM Greg Clayton <gclayton at apple.com <mailto:gclayton at apple.com>> wrote:
> > > > >
> > > > > > On Dec 12, 2016, at 1:59 PM, David Blaikie <dblaikie at gmail.com <mailto: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 <mailto: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 <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 <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/20170109/70a59034/attachment.html>