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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

There's a lot of this code that I don't have the knowledge to review, so I'm just focusing on the cosmetic aspects primarily.

Also, what do you think about splitting the changes up a bit. In particular, I think the section types testing for llvm-readobj could be done separately, along with the required code changes (e.g. the SHT_RISCV section type). There are probably other parts too.



================
Comment at: llvm/include/llvm/Support/ELFAttributeParser.h:22-23
+  StringRef Vendor;
+  std::map<unsigned, unsigned> Attributes;
+  std::map<unsigned, StringRef> AttributesStr;
+
----------------
Do these need to be ordered, or can they be `unordered_map` (or an LLVM equivalent)?


================
Comment at: llvm/include/llvm/Support/RISCVAttributeParser.h:27-28
+
+  void Unaligned_access(unsigned Tag, const uint8_t *Data, uint32_t &Offset);
+  void Stack_align(unsigned Tag, const uint8_t *Data, uint32_t &Offset);
+
----------------
These function names don't follow LLVM style (old or new). They should be `unalignedAccess` and `stackAlign`.


================
Comment at: llvm/include/llvm/Support/RISCVAttributes.h:28
+enum AttrType {
+  // Rest correspond to ELF/.riscv.attributes
+  Stack_align = 4,
----------------
Rest of what?


================
Comment at: llvm/include/llvm/Support/RISCVAttributes.h:29-34
+  Stack_align = 4,
+  Arch = 5,
+  Unaligned_access = 6,
+  Priv_spec = 8,
+  Priv_spec_minor = 10,
+  Priv_spec_revision = 12,
----------------
These names are not in LLVM style. Are they matching part of the specification? If not, please change them.


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:289
+  // Add features according to the ELF attribute section.
+  // If there is any unrecognized feature, ignore it.
+  RISCVAttributeParser Attributes;
----------------
is -> are
feature -> features
ignore it -> ignore tehm


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:292
+  if (Error E = getBuildAttributes(Attributes))
+    return Features; // Keep "c" feature if there is one from PlatformFlags.
+
----------------
from -> in


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:323
+
+      // FIXME: To take care version numbers.
+      Arch = Arch.drop_until([](char c) { return c == '_' || c == '\0'; });
----------------
To take care -> Handle


================
Comment at: llvm/lib/Support/ARMBuildAttrs.cpp:14
+const TagNameMap llvm::ARMBuildAttrs::ARMAttributeTags = {
+    {ARMBuildAttrs::File, "Tag_File"},
+    {ARMBuildAttrs::Section, "Tag_Section"},
----------------
The formatting changes here seem unrelated so shouldn't be made with this change, right?


================
Comment at: llvm/lib/Support/ELFAttributes.cpp:30
+
+int ELFAttrs::attrTypeFromString(StringRef Tag, TagNameMap Map) {
+  bool HasTagPrefix = Tag.startswith("Tag_");
----------------
Perhaps this should return an `Optional<T>` to avoid the -1 magic number?


================
Comment at: llvm/lib/Support/ELFAttributes.cpp:32
+  bool HasTagPrefix = Tag.startswith("Tag_");
+  for (auto TI = Map.begin(), TE = Map.end(); TI != TE; ++TI) {
+    StringRef TagName = TI->TagName;
----------------
Range-based for loop?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:121
+  // Size should have been accounted for already, now
+  // emit each field as its type (ULEB or String)
+  for (AttributeItem item : Contents) {
----------------
Nit: missing trailing full stop.


================
Comment at: llvm/test/MC/RISCV/invalid-attribute.s:1
+## Negative tests
+##  - Feed integer value to string type attribute
----------------
Negative tests:


================
Comment at: llvm/test/MC/RISCV/invalid-attribute.s:2-4
+##  - Feed integer value to string type attribute
+##  - Feed string value to integer type attribute
+##  - Invalid arch string
----------------
Full stop at end of each line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023