[llvm] r348811 - llvm-objcopy: Improve/simplify llvm::Error handling during notes iteration
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 28 12:49:20 PST 2019
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>
More information about the llvm-commits
mailing list