[lldb][AIX] Added XCOFF ParseSymtab handling

[DhruvSrivastavaX]

This PR is in reference to porting LLDB on AIX.

Link to discussions on llvm discourse and github:

1. https://discourse.llvm.org/t/port-lldb-to-ibm-aix/80640
2. Extending LLDB to work on AIX #101657
   The complete changes for porting are present in this draft PR:
   Extending LLDB to work on AIX #102601

Description:
Adding ParseSymtab logic after creating sections. It is able to handle both 32 and 64 bit symbols,
without the need to add template logic.

This is an incremental PR on top of my previous couple of XCOFF support commits.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Dhruv Srivastava (DhruvSrivastavaX)

Changes

This PR is in reference to porting LLDB on AIX.

Link to discussions on llvm discourse and github:

1. https://discourse.llvm.org/t/port-lldb-to-ibm-aix/80640
2. Extending LLDB to work on AIX #101657
   The complete changes for porting are present in this draft PR:
   Extending LLDB to work on AIX #102601

Description:
Adding ParseSymtab logic after creating sections. It is able to handle both 32 and 64 bit symbols,
without the need to add template logic.

This is an incremental PR on top of my previous couple of XCOFF support commits.

---

Full diff: https://github.com/llvm/llvm-project/pull/141577.diff

1 Files Affected:

• (modified) lldb/source/Plugins/ObjectFile/XCOFF/ObjectFileXCOFF.cpp (+65-1)

diff --git a/lldb/source/Plugins/ObjectFile/XCOFF/ObjectFileXCOFF.cpp b/lldb/source/Plugins/ObjectFile/XCOFF/ObjectFileXCOFF.cpp
index e629355cd40b9..7dfe6c362add4 100644
--- a/lldb/source/Plugins/ObjectFile/XCOFF/ObjectFileXCOFF.cpp
+++ b/lldb/source/Plugins/ObjectFile/XCOFF/ObjectFileXCOFF.cpp
@@ -188,7 +188,71 @@ AddressClass ObjectFileXCOFF::GetAddressClass(addr_t file_addr) {
   return AddressClass::eUnknown;
 }
 
-void ObjectFileXCOFF::ParseSymtab(Symtab &lldb_symtab) {}
+lldb::SymbolType MapSymbolType(llvm::object::SymbolRef::Type sym_type) {
+  if (sym_type == llvm::object::SymbolRef::ST_Function)
+    return lldb::eSymbolTypeCode;
+  else if (sym_type == llvm::object::SymbolRef::ST_Data)
+    return lldb::eSymbolTypeData;
+  else if (sym_type == llvm::object::SymbolRef::ST_File)
+    return lldb::eSymbolTypeSourceFile;
+  return lldb::eSymbolTypeInvalid;
+}
+
+void ObjectFileXCOFF::ParseSymtab(Symtab &lldb_symtab) {
+  SectionList *sectionList = GetSectionList();
+
+  for (const auto &symbol_ref : m_binary->symbols()) {
+    llvm::object::XCOFFSymbolRef xcoff_sym_ref(symbol_ref);
+    llvm::Expected<llvm::StringRef> name_or_err = xcoff_sym_ref.getName();
+    if (!name_or_err) {
+      consumeError(name_or_err.takeError());
+      continue;
+    }
+    llvm::StringRef symbolName = name_or_err.get();
+    // Remove the dot prefix for demangle
+    llvm::StringRef symbol_name =
+        symbolName.starts_with(".") ? symbolName.drop_front() : symbolName;
+    auto storageClass = xcoff_sym_ref.getStorageClass();
+    if (storageClass == XCOFF::C_HIDEXT && symbolName != "TOC") {
+      if (xcoff_sym_ref.getNumberOfAuxEntries() != 1)
+        continue;
+      auto aux_csect_or_err = xcoff_sym_ref.getXCOFFCsectAuxRef();
+      if (!aux_csect_or_err) {
+        consumeError(aux_csect_or_err.takeError());
+        continue;
+      }
+      const llvm::object::XCOFFCsectAuxRef csect_aux = aux_csect_or_err.get();
+      if (csect_aux.getStorageMappingClass() != XCOFF::XMC_PR ||
+          (m_binary->is64Bit() ? (csect_aux.getAuxType64() != XCOFF::AUX_CSECT)
+                               : false))
+        continue;
+    }
+
+    Symbol symbol;
+    symbol.GetMangled().SetValue(ConstString(symbol_name));
+
+    int16_t sectionNumber = xcoff_sym_ref.getSectionNumber();
+    size_t sectionIndex = static_cast<size_t>(sectionNumber - 1);
+    if (sectionNumber > 0 && sectionIndex < sectionList->GetSize()) {
+      lldb::SectionSP section_sp =
+          sectionList->GetSectionAtIndex(sectionNumber - 1);
+      if (!section_sp || section_sp->GetFileAddress() == LLDB_INVALID_ADDRESS)
+        continue;
+      lldb::addr_t file_addr = section_sp->GetFileAddress();
+      lldb::addr_t symbolValue = xcoff_sym_ref.getValue();
+      if (symbolValue < file_addr)
+        continue;
+      symbol.GetAddressRef() = Address(section_sp, symbolValue - file_addr);
+    }
+
+    Expected<llvm::object::SymbolRef::Type> sym_type_or_err =
+        symbol_ref.getType();
+    symbol.SetType(MapSymbolType(sym_type_or_err.get()));
+    printf("%s %d\n", symbol.GetName(), *sym_type_or_err);
+
+    lldb_symtab.AddSymbol(symbol);
+  }
+}
 
 bool ObjectFileXCOFF::IsStripped() { return false; }
 

[DhruvSrivastavaX]

Hi @labath @DavidSpickett
I have added the logic for ParseSymtab, but while trying to modify my test cases, I noticed that lldb-test is unable to emit Symbol data.
Is there any other way that i can use to add test for this code or am I missing something?
Although I did verify via prompt, that my symbols are getting added in the symtab.

[labath]

The code seems okay. I don't know anything about the format, so I don't know which parts are "obvious" and which not, but I'd encourage you to add comments where you think it might be useful.

You're right that lldb-test doesn't dump symbols. You should be able to test these changes with something like lldb %t -o "image dump symtab", but if you find that is missing some crucial piece of information, it would also be fine to extend lldb-test to dump the data you need.

[DavidSpickett]

I'm a bit bothered by how many continue are here, but they seem to be here for good reasons.

Once you've addressed this round of comments I'll read it again and see if it's clearer to me.

[DhruvSrivastavaX]

@labath

The code seems okay. I don't know anything about the format, so I don't know which parts are "obvious" and which not, but I'd encourage you to add comments where you think it might be useful.

Sure, I have added some comments for that now.

You're right that lldb-test doesn't dump symbols. You should be able to test these changes with something like lldb %t -o "image dump symtab", but if you find that is missing some crucial piece of information, it would also be fine to extend lldb-test to dump the data you need

Yes, I had checked it using image dump symtab itself, my symbols do get added.
As for the lldb-test not emitting symbols, I will create an issue for it so that we can start upgrading lldb-test as well.

[DhruvSrivastavaX]

Hi @DavidSpickett , I have addressed all your comments as well.

[labath]

The code is fine, but you should really add a test for it. You don't need to wait for lldb-test to grow symtab support. You can write a test that does something like:

# RUN: yaml2obj %s > %t
# RUN: %lldb %t -o "image dump symtab" -o exit | FileCheck %s
# CHECK: symtab contents

If you grep for "image dump symtab", you can find some existing tests to emulate.

I also feel I need to say something about comments. The most useful kind of comment is the one that answers the "why" question. Most of your comments try to answer the "what". Ideally the code should be clear enough that I can tell what it is doing without reading the comment. The part that's often not obvious is why is it doing that.

Taking this as an example:

    // Remove the dot prefix from symbol names before adding to symtab. '.text'
    // -> 'text'

The comment is not bad, but it's not that helpful as I can deduce that from the code. When reading this, the questions I have are:

• why are we doing that?
• who adds the "." and why?
• what would happen if we didn't do that?
• etc.

This isn't a hard requirement for the patch, but please keep this in mind.

[DhruvSrivastavaX]

Please ignore the above unrelated push logs. It got messed up due to some issues with our team's default branch last week.

[DhruvSrivastavaX]

Got it @labath, I have added the updates accordingly.
Hope this patch serves the purpose. 🙂

The code is fine, but you should really add a test for it. You don't need to wait for lldb-test to grow symtab support. You can write a test that does something like:

# RUN: yaml2obj %s > %t
# RUN: %lldb %t -o "image dump symtab" -o exit | FileCheck %s
# CHECK: symtab contents

If you grep for "image dump symtab", you can find some existing tests to emulate.

I also feel I need to say something about comments. The most useful kind of comment is the one that answers the "why" question. Most of your comments try to answer the "what". Ideally the code should be clear enough that I can tell what it is doing without reading the comment. The part that's often not obvious is why is it doing that.

Taking this as an example:

    // Remove the dot prefix from symbol names before adding to symtab. '.text'
    // -> 'text'

The comment is not bad, but it's not that helpful as I can deduce that from the code. When reading this, the questions I have are:

• why are we doing that?
• who adds the "." and why?
• what would happen if we didn't do that?
• etc.

This isn't a hard requirement for the patch, but please keep this in mind.