[PATCH] D60353: ELF: Add basic partition data structures and behaviours.
Peter Collingbourne via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 16 16:43:26 PDT 2019
pcc marked an inline comment as done.
pcc added inline comments.
================
Comment at: lld/ELF/Driver.cpp:1387
+
+ StringRef PartName = reinterpret_cast<const char *>(S->data().data());
+ for (Partition &Part : getPartitions()) {
----------------
grimar wrote:
> Seems you can use `toStringRef` (from llvm\ADT\StringExtras.h)?
Doesn't look like it, we need to stop at the null terminator here.
================
Comment at: lld/ELF/Driver.cpp:1396
+ if (NumPartitions == 254)
+ fatal("may not have more than 254 partitions");
+ ++NumPartitions;
----------------
grimar wrote:
> Should it be `error`?
With `fatal` we can avoid needing to worry about what happens in case of overflow. Since I'd expect this error to be uncommon I don't think we need to provide a great experience here.
================
Comment at: lld/ELF/Driver.cpp:1648
+ if (S->Type == SHT_LLVM_SYMPART) {
+ readSymbolPartitionSection<ELFT>(S);
+ return true;
----------------
grimar wrote:
> Will it be better to report errors somewhere here, so you could
> include the file name to error text?
The file name is accessible in `readSymbolPartitionSection` via `S->File` if we wanted to print it there, though I'm not sure if it would be useful since a user hitting this limit would probably want to see the names of all partitions, not just the one that overflowed. I'd probably punt on giving a more useful error message here for now unless/until there's evidence that people are hitting this case frequently.
================
Comment at: lld/ELF/SyntheticSections.cpp:3258
+Partition elf::Partitions[255];
+size_t elf::NumPartitions;
+
----------------
grimar wrote:
> Why not `std::vector<Partition>` Partitions?
Yes, I think that would be better, done. I'm not sure why I originally wrote it this way.
================
Comment at: lld/ELF/SyntheticSections.h:1063
+ StringRef Name;
+ unsigned getNumber() const;
+};
----------------
grimar wrote:
> Perhaps just inline the body?
We previously couldn't have inlined the body because it depended on the definition of `Partitions` below (and `Partitions` depends on the definition of `Partition`). But now that `Partitions` is a vector we can inline it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60353/new/
https://reviews.llvm.org/D60353
More information about the llvm-commits
mailing list