[RISCV-LLDB] RISCV feature attribute support and allows overriding additional(default) feature #147990

Resolved all review comments.

Author: santhoshe447

lldb/include/lldb/Utility/ArchSpec.h

@@ -537,13 +537,14 @@ class ArchSpec {
 
   void SetFlags(const std::string &elf_abi);
 
-  // Sets the target specific disassembly feature string
-  // for ELF disassembly.
-  void SetAdditionalDisassemblyFeatureStr(llvm::StringRef additional_features);
-
-  // Get the current target disassembly feature string.
-  llvm::StringRef GetAdditionalDisassemblyFeatureStr() const {
-    return llvm::StringRef(m_additional_disassembly_feature_str);
+  /// Sets the feature string that describes architecture specific capabilities
+  /// for use during instruction decoding.
+  void SetDisassemblyFeatures(std::string additional_features);
+
+  /// Returns the feature string used by the disassembler to decode instructions
+  /// based on target specific features.
+  llvm::StringRef GetDisassemblyFeatures() const {
+    return m_disassembly_feature_str;
   }
 
 protected:
@@ -557,8 +558,11 @@ class ArchSpec {
   // these are application specific extensions like micromips, mips16 etc.
   uint32_t m_flags = 0;
 
-  // Holds the additional disassembly feature string.
-  std::string m_additional_disassembly_feature_str;
+  /// Feature string containing architecture specific ISA(Instruction set
+  /// architecture) extensions present in the binary (e.g. "xqci" in RISCV).
+  /// This string is passed to the disassembler to enable accurate instruction
+  /// decoding based on the binary's supported ISA features.
+  std::string m_disassembly_feature_str;
 
   // Called when m_def or m_entry are changed.  Fills in all remaining members
   // with default values.

lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp

@@ -1451,9 +1451,8 @@ void DisassemblerLLVMC::UpdateFeatureString(llvm::StringRef additional_features,
   // Allow users to override default additional features.
   for (llvm::StringRef flag : llvm::split(additional_features, ",")) {
     flag = flag.trim();
-    if (flag.empty()) {
+    if (flag.empty())
       continue;
-    }
     // Check if disable (-c) is already present before adding default enable
     // (+c), since enable (+c) takes precedence whether disable (-c) appears
     // before or after in the feature string.
@@ -1621,11 +1620,11 @@ DisassemblerLLVMC::DisassemblerLLVMC(const ArchSpec &arch,
     features_str += "+a,+m,";
   }
 
-  const char *additional_features =
-      arch.GetAdditionalDisassemblyFeatureStr().data();
+  llvm::StringRef additional_features =
+      arch.GetDisassemblyFeatures();
   // Prepend the additional_features if it's not already in the features_str to
   // avoid duplicates.
-  if (additional_features) {
+  if (!additional_features.empty()) {
     UpdateFeatureString(additional_features, features_str);
   }
 

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

@@ -1287,34 +1287,34 @@ ObjectFileELF::RefineModuleDetailsFromNote(lldb_private::DataExtractor &data,
 
 void ObjectFileELF::ParseRISCVAttributes(DataExtractor &data, uint64_t length,
                                          ArchSpec &arch_spec) {
-  lldb::offset_t Offset = 0;
+  lldb::offset_t offset = 0;
 
-  uint8_t FormatVersion = data.GetU8(&Offset);
-  if (FormatVersion != llvm::ELFAttrs::Format_Version)
+  uint8_t format_version = data.GetU8(&offset);
+  if (format_version != llvm::ELFAttrs::Format_Version)
     return;
 
-  Offset = Offset + sizeof(uint32_t); // Section Length
-  llvm::StringRef VendorName = data.GetCStr(&Offset);
+  offset = offset + sizeof(uint32_t); // Section Length
+  llvm::StringRef vendor_name = data.GetCStr(&offset);
 
-  if (VendorName != "riscv")
+  if (vendor_name != "riscv")
     return;
 
   llvm::StringRef attr = "";
 
-  while (Offset < length) {
-    uint8_t Tag = data.GetU8(&Offset);
-    uint32_t Size = data.GetU32(&Offset);
+  while (offset < length) {
+    uint8_t Tag = data.GetU8(&offset);
+    uint32_t Size = data.GetU32(&offset);
 
     if (Tag != llvm::ELFAttrs::File || Size == 0)
       continue;
 
-    while (Offset < length) {
-      uint64_t Tag = data.GetULEB128(&Offset);
+    while (offset < length) {
+      uint64_t Tag = data.GetULEB128(&offset);
       if (Tag == llvm::RISCVAttrs::ARCH) {
-        attr = data.GetCStr(&Offset);
+        attr = data.GetCStr(&offset);
         break;
       } else {
-        data.GetULEB128(&Offset);
+        data.GetULEB128(&offset);
       }
     }
   }
@@ -1327,8 +1327,8 @@ void ObjectFileELF::ParseRISCVAttributes(DataExtractor &data, uint64_t length,
 
   for (const auto &ext : riscv_extensions) {
     if (!attr.empty() && attr.contains(ext) &&
-        !arch_spec.GetAdditionalDisassemblyFeatureStr().contains(ext)) {
-      arch_spec.SetAdditionalDisassemblyFeatureStr("+" + ext + ",");
+        !arch_spec.GetDisassemblyFeatures().contains(ext)) {
+      arch_spec.SetDisassemblyFeatures("+" + ext + ",");
     }
   }
 }
@@ -1618,8 +1618,10 @@ size_t ObjectFileELF::GetSectionHeaderInfo(SectionHeaderColl &section_headers,
           if (sheader.sh_type == SHT_ARM_ATTRIBUTES && section_size != 0 &&
               data.SetData(object_data, sheader.sh_offset, section_size) == section_size)
             ParseARMAttributes(data, section_size, arch_spec);
-        } else if (arch_spec.GetMachine() == llvm::Triple::riscv32 ||
-                   arch_spec.GetMachine() == llvm::Triple::riscv64) {
+        }
+
+        if (arch_spec.GetMachine() == llvm::Triple::riscv32 ||
+            arch_spec.GetMachine() == llvm::Triple::riscv64) {
           DataExtractor data;
           if (sheader.sh_type == SHT_RISCV_ATTRIBUTES && section_size != 0 &&
               (data.SetData(object_data, sheader.sh_offset, section_size) ==

lldb/source/Utility/ArchSpec.cpp

@@ -617,11 +617,11 @@ void ArchSpec::SetFlags(const std::string &elf_abi) {
   SetFlags(flag);
 }
 
-void ArchSpec::SetAdditionalDisassemblyFeatureStr(
-    llvm::StringRef additional_features) {
-  if (m_additional_disassembly_feature_str.find(additional_features.str()) ==
+void ArchSpec::SetDisassemblyFeatures(
+    std::string additional_features) {
+  if (m_disassembly_feature_str.find(additional_features) ==
       std::string::npos) {
-    m_additional_disassembly_feature_str += additional_features.str();
+    m_disassembly_feature_str += additional_features;
   }
 }
 

lldb/test/Shell/Disassemble/TestDisassembleRISCVXqciInstrutions.test

@@ -8,4 +8,3 @@
 # RUN: %lldb -b -o "disassemble -b -n main -Y -xqci,+c" %p/Inputs/riscv_xqci.out | FileCheck --check-prefix=CHECK-NOXQCI %s
 
 # CHECK-NOXQCI: <unknown>
-