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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Support/ELFAttributeParser.h:31
+
+  uint64_t ParseInteger(const uint8_t *Data, uint32_t &Offset);
+  StringRef ParseString(const uint8_t *Data, uint32_t &Offset);
----------------
MaskRay wrote:
> Use `camelCase` function names.
> 
> You can even start to use `variableName` for variables. Because this patch adds many new files. They don't need to get stuck with existing `VariableName` rule which is considered broken  https://lists.llvm.org/pipermail/llvm-dev/2019-February/129907.html
@MaskRay, re. variable names, whilst the existing scheme is indeed considered broken, I wasn't aware of any agreement that new code should follow the proposed new style?


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:308
+      default:
+        llvm_unreachable("Unknown extension in Arch attribute.");
+      case 'i':
----------------
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.


================
Comment at: llvm/lib/Support/ELFAttributes.cpp:23
+    if (TI->Attr == Attr) {
+      auto TagName = TI->TagName;
+      return HasTagPrefix ? TagName : TagName.drop_front(4);
----------------
Don't use `auto` unless the type is obvious from the RHS (e.g. due to a case)/is an iterator.


================
Comment at: llvm/lib/Support/ELFAttributes.cpp:33
+  for (auto TI = Map.begin(), TE = Map.end(); TI != TE; ++TI) {
+    auto TagName = TI->TagName;
+    if (TagName.drop_front(HasTagPrefix ? 0 : 4) == Tag) {
----------------
Ditto re. auto


================
Comment at: llvm/lib/Support/ELFAttributes.cpp:34-36
+    if (TagName.drop_front(HasTagPrefix ? 0 : 4) == Tag) {
+      return TI->Attr;
+    }
----------------
Unnecessary braces.


================
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];
----------------
MaskRay wrote:
> range-based for loop
What's the type of `item`? (Don't use `auto` here)


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:147
+  size_t Result = 0;
+  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:19
+private:
+  typedef enum {
+    HiddenAttribute,
----------------
Shouldn't this be `enum AttributeType { ... };`?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.h:57
+    // Create new attribute item.
+    Contents.push_back({NumericAttribute, Attribute, Value, std::string("")});
+  }
----------------
As @MaskRay said, `""` should be sufficient, i.e. without `std::string()`.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/attribute.s:3-4
+
+# RUN: llvm-mc -triple riscv32 -filetype asm -o - %s | FileCheck %s
+# RUN: llvm-mc -triple riscv64 -filetype asm -o - %s | FileCheck %s
+# RUN: llvm-mc -triple riscv32 -filetype obj -o - %s \
----------------
Why are these two checks here?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/attribute.s:5
+# RUN: llvm-mc -triple riscv64 -filetype asm -o - %s | FileCheck %s
+# RUN: llvm-mc -triple riscv32 -filetype obj -o - %s \
+# RUN:   | llvm-readobj --arch-specific - \
----------------
Rather than piping the object to stdout, create a real object so that you can use it again later (that way you don't need llvm-mc calls for each line).


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3531
+    switch (Type) {
+    case SHT_RISCV_ATTRIBUTES:
+      return "RISCV_ATTRIBUTES";
----------------
Test case for this code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023