[PATCH] D60353: ELF: Add basic partition data structures and behaviours.

[Peter Collingbourne via Phabricator via llvm-commits]

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