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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: lld/ELF/Driver.cpp:1396
+  if (NumPartitions == 254)
+    fatal("may not have more than 254 partitions");
+  ++NumPartitions;
----------------
grimar wrote:
> 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?
The `Partition` fields in `Symbol` and `InputSectionBase` are 8 bits wide, so we can't remove the limitation without making the fields wider. I'm not sure if we should do that because I don't think we should devote too much space in these common data structures to this feature without a good reason (e.g. users need >254 partitions).


================
Comment at: lld/ELF/Driver.cpp:1648
+    if (S->Type == SHT_LLVM_SYMPART) {
+      readSymbolPartitionSection<ELFT>(S);
+      return true;
----------------
grimar wrote:
> 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?
Makes sense now, but that means that we'd get the error even for files with `SHT_LLVM_SYMPART` sections that end up being ignored due to not being in dynsym. It would probably be better to print these errors in `readSymbolPartitionSection` once we know that we're going to create a partition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60353