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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/Support/ELFAttributeParser.h:51
+      : Vendor(Vendor), SW(nullptr), TagToStringMap(Map) {}
+
+  void parse(ArrayRef<uint8_t> Section, bool isLittle);
----------------
Add a public virtual destructor.


================
Comment at: llvm/include/llvm/Support/ELFBuildAttributes.h:22
+
+using TagNameMap = ArrayRef<struct TagNameItem>;
+
----------------
omit `struct`


================
Comment at: llvm/include/llvm/Support/ELFBuildAttributes.h:26
+
+enum AttrType { File = 1, Section = 2, Symbol = 3 };
+
----------------
These enum constants are defined in `binutils-gdb/bfd/elf-bfd.h`.

I am unsure this file should be named ELFBuildAttributes.h

ELFAttributes.h is probably fine.


================
Comment at: llvm/include/llvm/Support/RISCVAttributes.h:25
+
+extern TagNameMap RISCVAttributeTags;
+
----------------
Add `const`


================
Comment at: llvm/lib/Support/ELFBuildAttrs.cpp:14
+
+namespace llvm {
+namespace ELFBuildAttrs {
----------------
Delete `namespace llvm {`.


================
Comment at: llvm/lib/Support/RISCVAttributeParser.cpp:15
+
+namespace llvm {
+#define ATTRIBUTE_HANDLER(Attr_)                                               \
----------------
Delete `namespace llvm {`


================
Comment at: llvm/lib/Support/RISCVAttributes.cpp:14
+
+TagNameMap RISCVAttributeTags = {
+    {Stack_align, "Tag_stack_align"},
----------------
Add `const`


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1719
+  StringRef StringValue = "";
+  bool IsStringValue = false;
+
----------------
Use one variable for IsStringValue and IsIntegerValue.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:80
+                                               StringRef String) {
+  setAttributeItem(Attribute, String, /* OverwriteExisting= */ true);
+}
----------------
Inconsistent.

clang-format uses this form: `/*OverwriteExisting=*/true`


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:122
+  // emit each field as its type (ULEB or String)
+  for (size_t i = 0; i < Contents.size(); ++i) {
+    AttributeItem item = Contents[i];
----------------
range-based for loop


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h:21
+    enum {
+      HiddenAttribute = 0,
+      NumericAttribute,
----------------
Is `= 0` significant?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h:25
+      NumericAndTextAttributes
+    } Type;
+    unsigned Tag;
----------------
omit `Type`


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h:71
+
+    // Create new attribute item
+    AttributeItem Item = {AttributeItem::TextAttribute, Attribute, 0,
----------------
End with a period.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h:74
+                          std::string(Value)};
+    Contents.push_back(Item);
+  }
----------------
Use initializer lists. `Contents.push_back({.......})`


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp:30
+
+  std::string Arch = Twine("rv32").str();
+
----------------
Arch = "rv32"


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp:33
+  if (STI.hasFeature(RISCV::Feature64Bit))
+    Arch = Twine("rv64").str();
+
----------------
ditto


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp:93
+                                               StringRef String) {
+  OS << "\t.attribute\t" << Attribute << ", \"" << String << "\""
+     << "\n";
----------------
`"\"\n"`


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h:32
+  virtual void emitIntTextAttribute(unsigned Attribute, unsigned IntValue,
+                                    StringRef StringValue = "") = 0;
+
----------------
Can we avoid the default value?


================
Comment at: llvm/test/MC/RISCV/attribute-with-insts.s:5
+
+# RUN: llvm-mc -triple riscv32 -filetype=obj %s \
+# RUN: | llvm-objdump -triple riscv32 -d -M no-aliases - \
----------------
HsiangKai wrote:
> apazos wrote:
> > Kai, do you plan to add some llvm-readelf tests?
> I found that llvm-readelf is a symbolic link to llvm-readobj. So, there is no need to add test cases for llvm-readelf.
llvm-readobj and llvm-readelf have different output styles, so sometimes there may need two sets of tests. See test/tools/llvm-readobj/   GNU: LLVM: 

I haven't checked this patch, though.


================
Comment at: llvm/test/MC/RISCV/attribute-with-insts.s:6
+# RUN: llvm-mc -triple riscv32 -filetype=obj %s \
+# RUN: | llvm-objdump -triple riscv32 -d -M no-aliases - \
+# RUN: | FileCheck -check-prefix=CHECK-INST %s
----------------
Indent continuation lines by 2.


================
Comment at: llvm/test/MC/RISCV/attribute.s:1
+# Test llvm-mc could handle .attribute correctly.
+
----------------
`## ` for comments. They make comment lines stand out from RUN and CHECK lines.


================
Comment at: llvm/test/MC/RISCV/attribute.s:3
+
+# RUN: llvm-mc %s -triple=riscv32 -filetype=asm \
+# RUN:     | FileCheck %s
----------------
No need for a continuation line


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/attribute.test:69
+    Type: SHT_RISCV_ATTRIBUTES
+    Content: 4132000000726973637600012800000004100572763332693270305F6D3270305F613270305F6332703000060008020A000C00
----------------
Second to jhenderson's suggestion. This opaque content is not necessarily better than an assembly test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023