[llvm] r348811 - llvm-objcopy: Improve/simplify llvm::Error handling during notes iteration

[David Blaikie via llvm-commits]

Hey Lang - mind taking a look at this?

Seems the examples in the programmers manual aren't quite complete/correct.
If the example in the manual had a "return"/break/etc from inside the loop,
it would fail, because the Error would be unchecked (even though we know
there can't be any error if execution is inside the loop).

Does this patch look like the right fix/idiom/direction we should be
encouraging? Should we sure it up even further (commit comment mentions
this could potentially be further constrained to check for comparison with
the end iterator, maybe? but that might be too strong in some situations
(if a fallible iterator can be compared with something other than end - you
might do a sub-range iteration, in which case comparison with non-end maybe
sufficient to avoid looping again, and thus be a similar early-exit
criteria... ))

- Dave

On Mon, Dec 10, 2018 at 4:12 PM David Blaikie via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Author: dblaikie
> Date: Mon Dec 10 16:09:06 2018
> New Revision: 348811
>
> URL: http://llvm.org/viewvc/llvm-project?rev=348811&view=rev
> Log:
> llvm-objcopy: Improve/simplify llvm::Error handling during notes iteration
>
> Using an Error as an out parameter from an indirect operation like
> iteration as described in the documentation (
>
> http://llvm.org/docs/ProgrammersManual.html#building-fallible-iterators-and-iterator-ranges
> ) seems to be a little fussy - so here's /one/ possible solution, though
> I'm not sure it's the right one.
>
> Alternatively such APIs may be better off being switched to a standard
> algorithm style, where they take a lambda to do the iteration work that
> is then called back into (eg: "Error e = obj.for_each_note([](const
> Note& N) { ... });"). This would be safer than having an unwritten
> assumption that the user of such an iteration cannot return early from
> the inside of the function - and must always exit through the gift
> shop... I mean error checking. (even though it's guaranteed that if
> you're mid-way through processing an iteration, it's not in an  error
> state).
>
> Alternatively we'd need some other (the super untrustworthy/thing we've
> generally tried to avoid) error handling primitive that actually clears
> the error state entirely so it's safe to ignore.
>
> Fleshed this solution out a bit further during review - it now relies on
> op==/op!= comparison as the equivalent to "if (Err)" testing the Error.
> So just like an Error must be checked (even if it's in a success state),
> the Error hiding in the iterator must be checked after each increment
> (including by comparison with another iterator - perhaps this could be
> constrained to only checking if the iterator is compared to the end
> iterator? Not sure it's too important).
>
> So now even just creating the iterator and not incrementing it at all
> should still assert because the Error has not been checked.
>
> Reviewers: lhames, jakehehrlich
>
> Differential Revision: https://reviews.llvm.org/D55235
>
> Modified:
>     llvm/trunk/include/llvm/Object/ELFTypes.h
>     llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
>
> Modified: llvm/trunk/include/llvm/Object/ELFTypes.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Object/ELFTypes.h?rev=348811&r1=348810&r2=348811&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/Object/ELFTypes.h (original)
> +++ llvm/trunk/include/llvm/Object/ELFTypes.h Mon Dec 10 16:09:06 2018
> @@ -642,14 +642,19 @@ class Elf_Note_Iterator_Impl
>    // container, either cleanly or with an overflow error.
>    void advanceNhdr(const uint8_t *NhdrPos, size_t NoteSize) {
>      RemainingSize -= NoteSize;
> -    if (RemainingSize == 0u)
> +    if (RemainingSize == 0u) {
> +      // Ensure that if the iterator walks to the end, the error is
> checked
> +      // afterwards.
> +      *Err = Error::success();
>        Nhdr = nullptr;
> -    else if (sizeof(*Nhdr) > RemainingSize)
> +    } else if (sizeof(*Nhdr) > RemainingSize)
>        stopWithOverflowError();
>      else {
>        Nhdr = reinterpret_cast<const Elf_Nhdr_Impl<ELFT> *>(NhdrPos +
> NoteSize);
>        if (Nhdr->getSize() > RemainingSize)
>          stopWithOverflowError();
> +      else
> +        *Err = Error::success();
>      }
>    }
>
> @@ -657,6 +662,7 @@ class Elf_Note_Iterator_Impl
>    explicit Elf_Note_Iterator_Impl(Error &Err) : Err(&Err) {}
>    Elf_Note_Iterator_Impl(const uint8_t *Start, size_t Size, Error &Err)
>        : RemainingSize(Size), Err(&Err) {
> +    consumeError(std::move(Err));
>      assert(Start && "ELF note iterator starting at NULL");
>      advanceNhdr(Start, 0u);
>    }
> @@ -670,6 +676,10 @@ public:
>      return *this;
>    }
>    bool operator==(Elf_Note_Iterator_Impl Other) const {
> +    if (!Nhdr)
> +      (void)(bool)(*Other.Err);
> +    if (!Other.Nhdr)
> +      (void)(bool)(*Err);
>      return Nhdr == Other.Nhdr;
>    }
>    bool operator!=(Elf_Note_Iterator_Impl Other) const {
>
> Modified: llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp?rev=348811&r1=348810&r2=348811&view=diff
>
> ==============================================================================
> --- llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp (original)
> +++ llvm/trunk/tools/llvm-objcopy/ELF/ELFObjcopy.cpp Mon Dec 10 16:09:06
> 2018
> @@ -123,14 +123,9 @@ findBuildID(const object::ELFFile<ELFT>
>      if (Phdr.p_type != PT_NOTE)
>        continue;
>      Error Err = Error::success();
> -    if (Err)
> -      llvm_unreachable("Error::success() was an error.");
> -    for (const auto &Note : In.notes(Phdr, Err)) {
> -      if (Err)
> -        return std::move(Err);
> +    for (const auto &Note : In.notes(Phdr, Err))
>        if (Note.getType() == NT_GNU_BUILD_ID && Note.getName() ==
> ELF_NOTE_GNU)
>          return Note.getDesc();
> -    }
>      if (Err)
>        return std::move(Err);
>    }
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190120/de057e62/attachment.html>