[PATCH] D69709: [yaml2obj] - Add a way to describe the custom data that is not part of an output section.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 6 03:08:37 PST 2019
jhenderson added a comment.
I've not looked at the test yet. I'll get to that later.
================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:150
+ Addrsig,
+ SyntheticFiller
};
----------------
Maybe SyntheticFill or even just Fill instead of SyntheticFiller? This would better match the linker script directive. Everything in the output is technically Synthetic too...
================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:156
+
+ Section(SectionKind K) : Kind(K) {}
+ virtual ~Section();
----------------
Maybe we should rename this class to something more generic, like `Block` or `Piece` or similar? A fill isn't really a section, and calling it that might get a bit confusing. `RegularSection` could then just be called `Section`.
================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:195-198
+// SyntheticFiller is a synthetic section which might be used to write the
+// custom data around regular output sections. It does not present in the
+// sections header table, but it might affect the output file size and
+// program headers produced. Think about it as about piece of data.
----------------
"is a synthetic section which might be used to write the custom data around regular output sections" -> "is a block of data which is placed outside of sections"
"It does not present" -> "It is not present"
Then delete the "Think about it..." sentence.
================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:204
+ // We have to remember the offset of the filler, because it does not have
+ // a corresponding section header, like normal section. But we still might
+ // need this information when writing the output.
----------------
"like normal section" -> "like a section"
Delete "But" and "still"
================
Comment at: llvm/include/llvm/ObjectYAML/YAML.h:88
/// hex string) as binary to the given raw_ostream.
- void writeAsBinary(raw_ostream &OS) const;
+ /// N can be used to specify the number of the bytes to write.
+ void writeAsBinary(raw_ostream &OS, uint64_t N = UINT64_MAX) const;
----------------
"the number of the bytes" -> "the maximum number of bytes"
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:94
+struct PhdrEntry {
+ uint64_t ShOffset;
+ uint64_t ShSize;
----------------
grimar wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > `SegmentFragment` may be a better name. `Phdr` (program header) looks to me properties that describe the program header, but here this structure describes fragments that make up the segment content.
> > The name is inspired by `MCFragment`, a concept in MC. It has several subclasses such as MCFillFragment, MCPaddingFragment, etc.
> >
> > We probably should drop the `Segment` prefix from the name `SegmentFragment`. This structure can describe fragments of non-segment part as well, e.g. gaps between 2 non-SHF_ALLOC sections.
> Renamed to `Segment`. Also, renamed the method: `getEntriesForPhdr` -> `getPhdrSegments`.
Maybe the fields need renaming, as they aren't necessarily related to the sh_* section header fields? Probably just Offset, Size etc. What does Type mean for a non-section fragment?
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:204-205
+ void writeFiller(ELFYAML::SyntheticFiller &Filler,
+ ContiguousBlobAccumulator &CBA);
+
----------------
Filler -> Fill (in all cases)
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:673
+ELFState<ELFT>::getPhdrFragments(const ELFYAML::ProgramHeader &Phdr,
+ std::vector<typename ELFT::Shdr> &SHeaders) {
+ DenseMap<StringRef, ELFYAML::SyntheticFiller *> NameToFiller;
----------------
It doesn't look like `SHeaders` is modified. Can this be an `ArrayRef`?
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:674
+ std::vector<typename ELFT::Shdr> &SHeaders) {
+ DenseMap<StringRef, ELFYAML::SyntheticFiller *> NameToFiller;
+ for (const std::unique_ptr<ELFYAML::Section> &D : Doc.Descriptions)
----------------
NameToFiller -> NameToFill
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:695-697
+ StringRef ErrPrefix = NameToFiller.empty()
+ ? "unknown section referenced: '"
+ : "unknown section or filler referenced: '";
----------------
I think you can just simplify this down to the section or filler message always.
Also filler -> fill
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1218
+ raw_ostream &OS = CBA.getOSAndAlignedOffset(Filler.ShOffset, /*Align=*/1);
+ size_t Size = Filler.Pattern.binary_size();
+ if (!Size)
----------------
Maybe PatternSize would make the below loop a little easier to follow.
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1222
+
+ uint64_t I = 0;
+ for (; I + Size <= Filler.Size; I += Size)
----------------
Maybe `Written`?
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1225
+ Filler.Pattern.writeAsBinary(OS);
+ Filler.Pattern.writeAsBinary(OS, Filler.Size - I);
+}
----------------
Maybe a brief comment around here describing what exactly is written would be helpful. The variable renaming might make it superfluous though.
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1242
+ StringSet<> Seen;
+ for (const std::unique_ptr<ELFYAML::Section> &Sec : Doc.Descriptions) {
+ auto S = dyn_cast<ELFYAML::SyntheticFiller>(Sec.get());
----------------
I think it would be simpler to fold these two loops together, and then add one simple if to check whether to add to the DotShStrtab. You could use the `Seen` set to check for duplicates of both then, rather than needing to do extra look ups in `SN2I`. I'd also then tweak the error messages so that they say "section/fill" instead of just "section" or "CustomFiller".
================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1140
+ // When the Type string does not have a "SHT_" prefix, we know it is not a
+ // description of a regular ELF output section. Currently, we have a one
+ // special type named "CustomFiller". See comments for SyntheticFiller.
----------------
a one -> one
================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1144
+ IO.mapRequired("Type", StrType);
+ if (StrType == "CustomFiller") {
+ Section.reset(new ELFYAML::SyntheticFiller());
----------------
I'd just call it "Fill".
================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1359-1361
+ if (Sec->Pattern.binary_size() == 0 && Sec->Size != 0)
+ return "\"Pattern\" can't be empty when \"Size\" is greater than "
+ "0";
----------------
I think defaulting to a value of '0' for the pattern would be perfectly reasonable if there is no `Pattern` specified.
================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1485
IO.mapOptional("ProgramHeaders", Object.ProgramHeaders);
- IO.mapOptional("Sections", Object.Sections);
+ IO.mapOptional("Sections", Object.Descriptions);
IO.mapOptional("Symbols", Object.Symbols);
----------------
grimar wrote:
> MaskRay wrote:
> > I feel that using `Sections` for `Fragment`s may be confusing because `Fragment`s are not described by the section header table. Does a new field add a lot of complexity? If not, we can add a parallel YAML mapping key `Fragments`, and use `Fragments` instead of `Sections` when there is a need for a `Fragment` (i.e. when there is non-section data that needs describing).
> >
> > Opinions? @rupprecht @jhenderson @labath
> To clarify I understood the idea: do you mean that YAML writers should be able to use both "Sections" and "Fragments", where the latter will be just an alias for "Sections"? I.e to make both below to be valid?
>
> ```
> Fragments:
> - Section1
> - Fragmen1
> - Section2
> ```
>
> ```
> Sections:
> - Section1
> - Fragmen1
> - Section2
> ```
>
> With that since they are both optional we will need to add a check that they are not specified at the same time. Also, I guess most of users will just randomly use either "Sections" or "Fragments" because of common copy-paste approach. I'd prefer just to rename "Sections" to something if there are any concerns about the naming.
>
> When I wrote this patch I thought about this question though and came up to thinking about this part of YAML just like about a linker script's "SECTIONS" command. It might contain not only sections, but location counter moving, commands like "ASSERT" e.t.c, but it is still called "SECTIONS" and I probably do not find it confusing.
I'd keep it as "Sections". This is similar to linker scripts as @grimar says, but also more pragmatic, as I think the fill case is going to be rare, but when it is used, I think they should be mixed in order with the sections, as otherwise the order may be non-obvious.
================
Comment at: llvm/lib/ObjectYAML/YAML.cpp:42
if (!DataIsHexString) {
- OS.write((const char *)Data.data(), Data.size());
+ assert(N == UINT64_MAX || N <= Data.size());
+ OS.write((const char *)Data.data(), std::min(N, Data.size()));
----------------
I'd get rid of this assert. If we make `N` mean "the maximum size to write", it implies that less will be written if there is less to write, so the assertion becomes unnecessary.
================
Comment at: llvm/lib/ObjectYAML/YAML.cpp:47
+
+ assert((Data.size() % 2 == 0) && (N == UINT64_MAX || N <= Data.size() / 2));
+ for (uint64_t I = 0, E = std::min(N, Data.size() / 2); I != E; ++I) {
----------------
I'm not sure I follow the point of this assert? It probably needs an accompanying message.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69709/new/
https://reviews.llvm.org/D69709
More information about the llvm-commits
mailing list