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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/Driver.cpp:1387
+
+  StringRef PartName = reinterpret_cast<const char *>(S->data().data());
+  for (Partition &Part : getPartitions()) {
----------------
pcc wrote:
> 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.
Ah, right.


================
Comment at: lld/ELF/Driver.cpp:1396
+  if (NumPartitions == 254)
+    fatal("may not have more than 254 partitions");
+  ++NumPartitions;
----------------
pcc wrote:
> 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.
Ok, but now after `Partitions` became a vector, I guess you can just remove this 254 limitation?


================
Comment at: lld/ELF/Driver.cpp:1648
+    if (S->Type == SHT_LLVM_SYMPART) {
+      readSymbolPartitionSection<ELFT>(S);
+      return true;
----------------
pcc wrote:
> 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.
I meant you could report here errors from below like

"filename1: partitions cannot be used with the SECTIONS command"
"filename2: partitions cannot be used with the PHDRS command"

i.e.:

```
  llvm::erase_if(InputSections, [](InputSectionBase *S) {
    if (S->Type == SHT_LLVM_SYMPART) {
      <report such errors here>

      readSymbolPartitionSection<ELFT>(S);
      return true;
     }
```

So that you'd be able to report the file names of the objects which have the SHT_LLVM_SYMPART sections.
Does it make sense?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60353/new/

https://reviews.llvm.org/D60353