[lldb][AIX] Added XCOFF ParseSymtab handling #141577

DhruvSrivastavaX · 2025-05-27T10:50:31Z

This PR is in reference to porting LLDB on AIX.

Link to discussions on llvm discourse and github:

https://discourse.llvm.org/t/port-lldb-to-ibm-aix/80640
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 · 2025-05-27T10:51:05Z

@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:

https://discourse.llvm.org/t/port-lldb-to-ibm-aix/80640
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 · 2025-05-27T10:53:34Z

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.

labath · 2025-05-29T08:00:05Z

lldb/source/Plugins/ObjectFile/XCOFF/ObjectFileXCOFF.cpp

+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)


https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

Could also be written as:

switch (symboltype): case foo: return bla; ... default: ...

lldb/source/Plugins/ObjectFile/XCOFF/ObjectFileXCOFF.cpp

labath · 2025-05-29T08:12:14Z

lldb/source/Plugins/ObjectFile/XCOFF/ObjectFileXCOFF.cpp

+      if (csect_aux.getStorageMappingClass() != XCOFF::XMC_PR ||
+          (m_binary->is64Bit() ? (csect_aux.getAuxType64() != XCOFF::AUX_CSECT)
+                               : false))
+        continue;


I'd split this into two conditions:

if (csect_aux.getStorageMappingClass() != XCOFF::XMC_PR) continue; if (m_binary->is64Bit() && csect_aux.getAuxType64() != XCOFF::AUX_CSECT) continue;

Yes please, my reaction was "this looks very clever but needs to be refactored" :)

Yes, my effort to avoid another continue 🙂 .
But I have modified that now.

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.

DavidSpickett · 2025-05-29T12:21:28Z

lldb/source/Plugins/ObjectFile/XCOFF/ObjectFileXCOFF.cpp

@@ -188,7 +188,74 @@ 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) {


Mark this static, because it's only used in this file.

DavidSpickett · 2025-05-29T12:22:46Z

lldb/source/Plugins/ObjectFile/XCOFF/ObjectFileXCOFF.cpp

+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)


Could also be written as:

switch (symboltype): case foo: return bla; ... default: ...

DavidSpickett · 2025-05-29T12:25:29Z

lldb/source/Plugins/ObjectFile/XCOFF/ObjectFileXCOFF.cpp

+    if (!name_or_err) {
+      consumeError(name_or_err.takeError());
+      continue;
+    }


The use of continue is ok but I find it harder to read when there is:

continue; immediately more code here

Please put a blank line after each continue or block that contains the continue, like:

if (bla) { continue; } rest of code goes here

DavidSpickett · 2025-05-29T12:28:05Z

lldb/source/Plugins/ObjectFile/XCOFF/ObjectFileXCOFF.cpp

+      continue;
+    }
+    llvm::StringRef symbolName = name_or_err.get();
+    // Remove the dot prefix for demangle


This dot prefix is what exactly, is it an expectation of XCOFF in general? Seems like only some symbols will have it.

You say "for demangle" but it's not clear where that happens or if you mean remove it so that some other thing later can do it. If it does happen in this function perhaps clarify:

// Remove the dot prefix so that we can demangle it.

Now the link between action and purpose is clearer.

DavidSpickett · 2025-05-29T12:29:40Z

lldb/source/Plugins/ObjectFile/XCOFF/ObjectFileXCOFF.cpp

+      if (csect_aux.getStorageMappingClass() != XCOFF::XMC_PR ||
+          (m_binary->is64Bit() ? (csect_aux.getAuxType64() != XCOFF::AUX_CSECT)
+                               : false))
+        continue;


Yes please, my reaction was "this looks very clever but needs to be refactored" :)

DavidSpickett · 2025-05-29T12:32:05Z

lldb/source/Plugins/ObjectFile/XCOFF/ObjectFileXCOFF.cpp

+    Symbol symbol;
+    symbol.GetMangled().SetValue(ConstString(symbol_name));
+
+    int16_t sectionNumber = xcoff_sym_ref.getSectionNumber();


I assume this is defined by some XCOFF standard to start at 1? So the line below isn't going to be doing 0 - 1, at minimum it will be 1-1.

I would add a clarifying comment here:

// Note that XCOFF section numbers start from 1.

Assuming that's correct.

But later you say:

if (sectionNumber > 0

and it's a signed int, so it could be negative.

So if it can be 0, then sectionIndex could become 0xffffffffffffffff. Now maybe it doesn't get used in that scenario but if it did, it would not do good things.

I would move the if sectionNumber > 0 up to before you calculate the section index. Then you aren't creating random sectionIndex values.

Plus, it should be clearer then what range of values we expect from sectionNumber.

DavidSpickett · 2025-05-29T12:36:29Z

lldb/source/Plugins/ObjectFile/XCOFF/ObjectFileXCOFF.cpp

+    size_t sectionIndex = static_cast<size_t>(sectionNumber - 1);
+    if (sectionNumber > 0 && sectionIndex < sectionList->GetSize()) {
+      lldb::SectionSP section_sp =
+          sectionList->GetSectionAtIndex(sectionNumber - 1);


sectionNumber -1 is just the sectionIndex you already made, I think the parameter type is also size_t so you can just use it here.

DhruvSrivastavaX · 2025-05-29T12:48:46Z

@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.

Added XCOFF ParseSymtab handling

a62cd7b

DhruvSrivastavaX requested a review from JDevlieghere as a code owner May 27, 2025 10:50

llvmbot added the lldb label May 27, 2025

DhruvSrivastavaX requested review from DavidSpickett and labath May 27, 2025 10:50

cleanup

21f5a39

DhruvSrivastavaX force-pushed the aix-xcoff-parsesymtab branch from 3b425c9 to 21f5a39 Compare May 28, 2025 12:42

labath reviewed May 29, 2025

View reviewed changes

Addressed comments

4d63189

DavidSpickett reviewed May 29, 2025

View reviewed changes

[lldb][AIX] Added XCOFF ParseSymtab handling #141577

Are you sure you want to change the base?

[lldb][AIX] Added XCOFF ParseSymtab handling #141577

Uh oh!

Conversation

DhruvSrivastavaX commented May 27, 2025

Uh oh!

llvmbot commented May 27, 2025

Uh oh!

DhruvSrivastavaX commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

labath left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidSpickett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DhruvSrivastavaX commented May 29, 2025

Uh oh!

Uh oh!

DhruvSrivastavaX commented May 27, 2025 •

edited

Loading