[PATCH] D70759: [llvm-symbolizer] Support debug file lookup using build ID

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:303
+        return N.getDesc();
+  }
+  return {};
----------------
lhames wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > > vsk wrote:
> > > > Does `Err` need to be checked here, after the loop?
> > > > 
> > > > From `notes_begin`:
> > > > 
> > > >   /// \param Err [out] an error to support fallible iteration, which should
> > > >   ///  be checked after iteration ends.
> > > > 
> > > Looks like it to me.
> > > 
> > > A test case would be good/necessary, though - @lhames - you wouldn't happen to know a good way to produce an object file with this problem, by chance?
> > Also is there anything to be done about this idiom to make it more robust like other uses of llvm::Error? So that this usage would fail even in Success-only executions.
> I think it should already fail if you don't check Err, even on success. The caveat is that you would have to exit the bottom of the loop: If you always leave via that return statement (which you might have if that's what you were testing) then you aren't forced to check the error, because returns from inside the loop body are guaranteed to be safe. Making safe returns not require checking was actually one of the main reasons we developed the fallible iterator wrapper in the first place, if memory serves. :) 
That does sound vaguely familiar - though whichever case it was (that we designed it this way intentionally or accidentally) - reckon it might be worth revisiting given how easy it is to write code like this that sort of defeats the "must always check" kind of design of Error?

Hmm - I was going to say that I think I'd be OK saying you can't return early from a fallible iteration - but given range-based-for loops make it hard to add new conditions into your loop to exit early when you've found the thing you want, maybe that's not ideal either. Then I end up back at the functor based iteration schemes, rather than exposing a full iterator/range?

BUt yeah, I guess for this code, a test case that doesn't have a BUILD_ID should suffice... - any ideas if yaml2obj is the right tool for that? Or other ways to construct such objects for testing this sort of thing?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70759/new/

https://reviews.llvm.org/D70759