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

[David Blaikie via llvm-commits]

Ping

On Sun, Jan 20, 2019 at 1:40 PM David Blaikie <dblaikie at gmail.com> wrote:

> 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/20190128/08f42639/attachment.html>