[PATCH] D66426: [lld] Enable a watermark of loadable sections to be generated and placed in a note section

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

I suggest adding a dumper to llvm-readobj and then adding a test to `test/ELF/partition-notes.s`



================
Comment at: lld/ELF/SyntheticSections.cpp:342
 
+lld::elf::WatermarkSection::WatermarkSection()
+:SyntheticSection(SHF_ALLOC, SHT_NOTE, 4, ".note.llvm.watermark")
----------------
Delete `lld::elf::`.


================
Comment at: lld/ELF/SyntheticSections.cpp:347
+void lld::elf::WatermarkSection::writeTo(uint8_t *buf) {
+  write32(buf, 5);                     // Name size
+  write32(buf + 4, watermarkSize);     // Content size
----------------
5 -> 8, otherwise it is incorrect to use `watermarkBuf = buf + 20;`


================
Comment at: lld/ELF/SyntheticSections.cpp:354
+
+ void lld::elf::WatermarkSection::writeWatermark(llvm::ArrayRef<uint8_t> buf) {
+  assert(buf.size() == watermarkSize);
----------------
Delete `lld::elf::`

Delete `llvm::`


================
Comment at: lld/ELF/Writer.cpp:642
 
+  // Compute and fill the watermark note.
+  writeWatermark();
----------------
Backfill .note.llvm.watermark section content. This is similar to .note.gnu.build-id.


================
Comment at: lld/ELF/Writer.cpp:2689
+
+  size_t hashSize = mainPart->buildId->hashSize;
+  std::vector<uint8_t> buildId(hashSize);
----------------
inline the only use of the variable


================
Comment at: lld/ELF/Writer.cpp:2692
+  llvm::ArrayRef<uint8_t> buf{Out::bufferStart, size_t(fileSize)};
+  std::vector<llvm::ArrayRef<uint8_t>> parts = {buf};
+
----------------
Delete `parts`


================
Comment at: lld/ELF/Writer.cpp:2716
+
+  if (first > last)
+    return;
----------------
`if (first >= last)` might be better (WriteHash asserts `first < last` though I haven't found a case where first can be equal to last)


================
Comment at: lld/test/ELF/watermark.s:1
+## Test that a watermark is generated placed in the correct section with the
+## correct alignment when using --watermark. Check also that the watermark can
----------------
`generated placed`?


================
Comment at: lld/test/ELF/watermark.s:5
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: ld.lld %t -o %t.default
----------------
`-triple=x86_64 %s -o %t.o` (this is generic, not Linux specific). Use .o for object files.


================
Comment at: lld/test/ELF/watermark.s:14
+# RUN: ld.lld --watermark %t -o %t.watermark
+# RUN: llvm-readobj -S %t.watermark | FileCheck -check-prefix=SECTION %s
+# RUN: llvm-objdump -s %t.watermark | FileCheck --strict-whitespace -check-prefix=CONTENT %s
----------------
Consider `llvm-readelf -S`. Its output is concise.


================
Comment at: lld/test/ELF/watermark.s:15
+# RUN: llvm-readobj -S %t.watermark | FileCheck -check-prefix=SECTION %s
+# RUN: llvm-objdump -s %t.watermark | FileCheck --strict-whitespace -check-prefix=CONTENT %s
+
----------------
`llvm-readelf -x .note.llvm.watermark` (Prefer llvm-readelf -x over llvm-objdump -s`)


================
Comment at: lld/test/ELF/watermark.s:23
+# SECTION-NEXT: Address:
+# SECTION-NEXT: Offset: [[_:0x[0-9A-Z]*(0|4|8|C)$]]
+# SECTION-NEXT: Size: 28
----------------
[048C]


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

https://reviews.llvm.org/D66426