[PATCH] D74023: [RISCV] ELF attribute section for RISC-V

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/section-types.test:11
+
+# GNU:      [ 0]                   NULL
+# GNU-NEXT: [ 1] .riscv.attributes RISCV_ATTRIBUTES
----------------
You don't need this line.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/section-types.test:23
+    Type: SHT_RISCV_ATTRIBUTES
+    Content: 4132000000726973637600012800000004100572763332693270305F6D3270305F613270305F6332703000060008020A000C00
----------------
You don't need the Content blob, as the contents of the section aren't important for printing the section header.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/unknown-arch-attr.test:4
+## The expected behavior is to ignore the unrecognized arch feature and
+## continue to process following arch features.
+##
----------------
process the following


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/unknown-arch-attr.test:6
+##
+## I provide "rv32i2p0_x1p0_m2p0_a2p0" arch feature into the object file.
+## "x1p0" is an unrecognized architecture extension. llvm-objdump will ignore
----------------
Write this in the third person, describing what is in the object file, i.e. something like "The object file has the "rv32i2p0_x1p0_m2p0_a2p0" arch feature."


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/unknown-arch-attr.test:22-27
+# CHECK-OBJ32:      Format: ELF32-riscv
+# CHECK-OBJ32-NEXT: Arch: riscv32
+# CHECK-OBJ32-NEXT: AddressSize: 32bit
+# CHECK-OBJ64:      Format: ELF64-riscv
+# CHECK-OBJ64-NEXT: Arch: riscv64
+# CHECK-OBJ64-NEXT: AddressSize: 64bit
----------------
Are these bits relevant to the thing under test?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/unknown-arch-attr.test:30
+# BOTH-NEXT:        Value: rv32i2p0_x1p0_m2p0_a2p0
+# CHECK-OBJDUMP:    mul a0, a1, a2
+
----------------
Maybe change this prefix to just `DISASM:`


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/unknown-arch-attr.test:42
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+    Content: 3385C502
+  - Name:    .riscv.attributes
----------------
I'm guessing this test can't be assembly because it needs to use an unsupported feature? If so, please comment this and the other content block below describing what the content represents. E.g. here you might put something like `mov %rsp,3` (or whatever this actually is).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023