[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:
> > > 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).
> Ok, thanks for the explanation! Perhaps it worst to add this information to the documentation in D60242 then? It wasn't obvious for me where this limitation technically comes from.
I added a note to the documentation as well as a comment here.


================
Comment at: lld/ELF/InputSection.cpp:1091
   Alignment = std::max(Alignment, Other->Alignment);
+  if (Partition != Other->Partition) {
+    Partition = 1;
----------------
MaskRay wrote:
> This probably needs a comment.
Done. Also realised that I don't have a test for this code, so I added one.


================
Comment at: lld/ELF/MarkLive.cpp:202
 
-  if (Sec->Live)
+  // Set Sec->Partition to the minimum of Partition and Sec->Partition in the
+  // following lattice: 1 < other < 0. If Sec->Partition doesn't change, we
----------------
MaskRay wrote:
> minimum -> meet?
Hmm, that's the correct set theoretical term, but "minimum" seems more intuitive to me. I decided to reword it as `meet (i.e. the "minimum")`


================
Comment at: lld/test/ELF/partitions.s:4
+// RUN: ld.lld %t.o -o %t --export-dynamic --gc-sections
+// RUN: llvm-readobj -sections -symbols -elf-output-style=GNU %t | FileCheck %s
+
----------------
MaskRay wrote:
> `-symbols` -> `-dyn-syms`?
> 
> e.g. `llvm-readelf -S -dyn-syms %t`
That would not be sufficient to test the property that we're interested in here because `f[3-6]` are not exported.


================
Comment at: lld/test/ELF/partitions.s:32
+_start:
+call f3
+
----------------
MaskRay wrote:
> Indent instructions?
I don't think we have a consistent style for whether or not to indent, so I left things as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60353