Page MenuHomePhabricator

[llvm-objcopy][MachO][NFC] Allow section to own its contents
AbandonedPublic

Authored by seiya on Dec 11 2019, 1:59 AM.

Details

Summary

This patch allows section to have owned section contents instead of a reference. This feature is necessary when we want to create a new section in the object (e.g., --add-section).

I'll commit this patch along with D66283 since this patch does not include test cases for newly added code.

Diff Detail

Event Timeline

seiya created this revision.Dec 11 2019, 1:59 AM
Herald added a project: Restricted Project. · View Herald Transcript
jhenderson added inline comments.Dec 11 2019, 3:08 AM
llvm/tools/llvm-objcopy/MachO/Object.h
42

Since you're storing the owned content as a std::vector<uint8_t>, it may make more sense for Content to be an ArrayRef rather than a StringRef.

seiya updated this revision to Diff 233494.Dec 11 2019, 9:24 PM

Addressed review comments.

seiya marked an inline comment as done.Dec 11 2019, 9:24 PM
alexshap added inline comments.Dec 11 2019, 10:26 PM
llvm/tools/llvm-objcopy/MachO/Object.h
43

frankly speaking i am somehow not sure this is the best possible solution.
Having these two fields + sort of duplication of logic / code size increase ..

Maybe we can consider some alternatives ?

The first thing which comes on my mind -
use StringSaver NewSectionsContents
(for example, it can be a field of Object)
and keep using ArrayRef / StringRef inside Section.
But there are other options as well.

What do you think ? (maybe I'm missing something)

jhenderson added inline comments.Dec 12 2019, 2:08 AM
llvm/tools/llvm-objcopy/MachO/Object.h
43

A StringSaver sounds like an interesting idea. I think it's worth exploring.

seiya marked an inline comment as done.Dec 12 2019, 5:58 PM
seiya added inline comments.
llvm/tools/llvm-objcopy/MachO/Object.h
43

Sounds good to me too. I'll try it out.

seiya abandoned this revision.Dec 12 2019, 9:11 PM

Merged into D66283: I think it does not have to be a separate patch because using StringSaver simplified what I wanted to do in this patch.