This is an archive of the discontinued LLVM Phabricator instance.

Set error status in ObjectFile::LoadInMemory if it is not set
Needs ReviewPublic

Authored by tatyana-krasnukha on Nov 13 2017, 10:45 AM.

Details

Reviewers
clayborg
abidh
Summary

When breakpoints intersect a section, process->WriteMemory returns 0 without setting the error, LoadInMemory breaks execution and returns this error, but client code treat it as success.

Just added setting of error status for this case.

Applied clang-format.

Diff Detail

Repository
rL LLVM

Event Timeline

clayborg requested changes to this revision.Nov 14 2017, 8:40 AM
clayborg added a subscriber: clayborg.

Before working on a file for a diff you will submit, clang format it first, and then check it in. No need for review on clang format only changes.

So please clang format first and check in those change and update this with only the changes you intend to make to the functionality of the code.

This revision now requires changes to proceed.Nov 14 2017, 8:40 AM
tatyana-krasnukha edited edge metadata.

Removed clang-format changes.

abidh added inline comments.Nov 15 2017, 2:59 AM
source/Symbol/ObjectFile.cpp
695

If WriteMemory is not setting the error in some case, it seem more logical to properly handle it there instead of putting this specific error message here.

source/Symbol/ObjectFile.cpp
695

Process::WriteMemory doesn't treat this case as error. This depends on context of usage. I wanted to do as you propose, but was afraid for breaking all other use cases...

ping

Greg, Abid, if you disagree with the changes, let me know and I'll close the revision.

tatyana-krasnukha removed rL LLVM as the repository for this revision.Jan 22 2018, 10:59 AM
tatyana-krasnukha removed a subscriber: llvm-commits.

I am not sure we can say for sure that a breakpoint intersected with the write here? I would rather just have an error saying something like "only %u of %u bytes in section %s were written". Extra credit for checking if there are overlapping breakpoints and appending "(try disabling all breakpoints and loading again)" to the error message.

tatyana-krasnukha added a comment.EditedJan 22 2018, 11:38 AM

Yes, I see... There is some inconsistency between status and returned value. That is why I wondered about this https://reviews.llvm.org/D39967#984065

And yes, if the write memory can only write fewer byte than requested, it won't be an error if at least some bytes were written

abidh resigned from this revision.Oct 13 2020, 1:09 PM