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

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 27 01:47:37 PST 2020


jhenderson added inline comments.


================
Comment at: lld/test/ELF/riscv-attributes.s:12
+
+#CHECK: Tag: 4
+#CHECK-NEXT: Value: 16
----------------
`#CHECK:` -> `# CHECK:` (same for the CHECK-NEXT lines).

Also, here and at the similar lines below, please add a few spaces so that `Tag` lines up with `Value`, i.e.

```
# CHECK:      Tag: 4
# CHECK-NEXT: Value: 16
```


================
Comment at: lld/test/ELF/riscv-attributes.s:34-39
+.attribute  Tag_stack_align, 16
+.attribute arch, "rv32i2p0_m2p0_f2p0_d2p0"
+.attribute unaligned_access, 0
+.attribute  Tag_priv_spec, 2
+.attribute  Tag_priv_spec_minor, 0
+.attribute  Tag_priv_spec_revision, 0
----------------
Normalise the number of spaces after .attribtue please, to be consistent.


================
Comment at: lld/test/ELF/riscv-attributes.s:43-44
+_start:
+  .globl func
+  call func
----------------
Do you need these? Is the `call` relevant, or can it just be `nop`?


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:308
+      default:
+        llvm_unreachable("Unknown extension in Arch attribute.");
+      case 'i':
----------------
jhenderson wrote:
> HsiangKai wrote:
> > MaskRay wrote:
> > > The `defualt` branch is still reachable, thus it is incorrect to use llvm_unreachable.
> > > You may want to return an Error.
> > Encoding part only accepts `iemafdc`. Decoding part should expect these features only. Default case should be unreachable.
> Is it unreachable if somebody provides an invalid binary (i.e. with something incorrectly encoded)? Decoding needs to handle invalid binaries without crashing on an llvm_unreachable.
Please add a test case for an unknown letter.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:160-162
+  bool getFeatureBits(uint64_t feature) {
+    return getSTI().getFeatureBits()[feature];
+  }
----------------
The rest of this code uses upper-case variable names. We should standardise on one or the other in the file throughout, I think.


================
Comment at: llvm/test/MC/RISCV/attribute-with-option.s:10
+
+#CHECK: Tag: 5
+#CHECK-NEXT: TagName: arch
----------------
Same comment as earlier test, re. spacing.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/attr-section-header.s:1
+## Show attribute section header correctly.
+
----------------
It might make sense to name this test slightly more generically, i.e. something like section-type.test, and update the comment accordingly. That way, if more section types are supported in the future, this will be a place that can be extended. See also ELF/section-type.test which handles generic sections.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/attr-section-header.s:7-8
+
+.attribute  Tag_arch, "rv32i2p0"
+#CHECK: .riscv.attributes RISCV_ATTRIBUTES
----------------
This test should probably use yaml2obj to generate the table. It should also show that the section type is printed correctly for both llvm-readelf and llvm-readobj.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/attribute.s:15
+.attribute  Tag_stack_align, 16
+#CHECK-OBJ: Tag: 4
+#CHECK-OBJ-NEXT: Value: 16
----------------
Same comments re. spacing.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/attribute.test:69
+    Type: SHT_RISCV_ATTRIBUTES
+    Content: 4132000000726973637600012800000004100572763332693270305F6D3270305F613270305F6332703000060008020A000C00
----------------
MaskRay wrote:
> Second to jhenderson's suggestion. This opaque content is not necessarily better than an assembly test.
Seems like this comment still hasn't been addressed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023





More information about the llvm-commits mailing list