llvm.org GIT mirror llvm / 4dd6cea
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 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@348811 91177308-0d34-0410-b5e6-96231b3b80d8 David Blaikie 11 months ago
2 changed file(s) with 13 addition(s) and 8 deletion(s). Raw diff Collapse all Expand all
641641 // container, either cleanly or with an overflow error.
642642 void advanceNhdr(const uint8_t *NhdrPos, size_t NoteSize) {
643643 RemainingSize -= NoteSize;
644 if (RemainingSize == 0u)
644 if (RemainingSize == 0u) {
645 // Ensure that if the iterator walks to the end, the error is checked
646 // afterwards.
647 *Err = Error::success();
645648 Nhdr = nullptr;
646 else if (sizeof(*Nhdr) > RemainingSize)
649 } else if (sizeof(*Nhdr) > RemainingSize)
647650 stopWithOverflowError();
648651 else {
649652 Nhdr = reinterpret_cast *>(NhdrPos + NoteSize);
650653 if (Nhdr->getSize() > RemainingSize)
651654 stopWithOverflowError();
655 else
656 *Err = Error::success();
652657 }
653658 }
656661 explicit Elf_Note_Iterator_Impl(Error &Err) : Err(&Err) {}
657662 Elf_Note_Iterator_Impl(const uint8_t *Start, size_t Size, Error &Err)
658663 : RemainingSize(Size), Err(&Err) {
664 consumeError(std::move(Err));
659665 assert(Start && "ELF note iterator starting at NULL");
660666 advanceNhdr(Start, 0u);
661667 }
669675 return *this;
670676 }
671677 bool operator==(Elf_Note_Iterator_Impl Other) const {
678 if (!Nhdr)
679 (void)(bool)(*Other.Err);
680 if (!Other.Nhdr)
681 (void)(bool)(*Err);
672682 return Nhdr == Other.Nhdr;
673683 }
674684 bool operator!=(Elf_Note_Iterator_Impl Other) const {
122122 if (Phdr.p_type != PT_NOTE)
123123 continue;
124124 Error Err = Error::success();
125 if (Err)
126 llvm_unreachable("Error::success() was an error.");
127 for (const auto &Note : In.notes(Phdr, Err)) {
128 if (Err)
129 return std::move(Err);
125 for (const auto &Note : In.notes(Phdr, Err))
130126 if (Note.getType() == NT_GNU_BUILD_ID && Note.getName() == ELF_NOTE_GNU)
131127 return Note.getDesc();
132 }
133128 if (Err)
134129 return std::move(Err);
135130 }