labath updated this revision to Diff 182254.
labath marked 7 inline comments as done.
labath added a comment.
Thanks for the review. I've refactored the code to separate (and centralize) the
breakpad parsing from the part which does presents the information to lldb.
I've done this slightly differently than suggested (the new file is called
BreakpadRecords, as that's how breakpad calls them, and there as separate class
for each record type instead of just a function), but I believe it addresses the
root problem.
The main reason I created classes for everything is because I thought I'd make
the iterator class return those. I may still do that at some point but now it
did not seem worth it now because it makes it harder to report parsing errors,
and it did not make the call sites much cleaner.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56590/new/
https://reviews.llvm.org/D56590
Files:
lit/SymbolFile/Breakpad/Inputs/symtab.syms
lit/SymbolFile/Breakpad/symtab.test
source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp
Index: unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp
===================================================================
--- unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp
+++ unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp
@@ -51,12 +51,27 @@
EXPECT_EQ(llvm::None, InfoRecord::parse("INFO CODE_ID"));
}
+TEST(FuncRecord, parse) {
+ EXPECT_EQ(FuncRecord(true, 0x47, 0x7, 0x8, "foo"),
+ FuncRecord::parse("FUNC m 47 7 8 foo"));
+ EXPECT_EQ(FuncRecord(false, 0x47, 0x7, 0x8, "foo"),
+ FuncRecord::parse("FUNC 47 7 8 foo"));
+
+ EXPECT_EQ(llvm::None, FuncRecord::parse("PUBLIC 47 7 8 foo"));
+ EXPECT_EQ(llvm::None, FuncRecord::parse("FUNC 47 7 8"));
+ EXPECT_EQ(llvm::None, FuncRecord::parse("FUNC 47 7"));
+ EXPECT_EQ(llvm::None, FuncRecord::parse("FUNC 47"));
+ EXPECT_EQ(llvm::None, FuncRecord::parse("FUNC m"));
+ EXPECT_EQ(llvm::None, FuncRecord::parse("FUNC"));
+}
+
TEST(PublicRecord, parse) {
EXPECT_EQ(PublicRecord(true, 0x47, 0x8, "foo"),
PublicRecord::parse("PUBLIC m 47 8 foo"));
EXPECT_EQ(PublicRecord(false, 0x47, 0x8, "foo"),
PublicRecord::parse("PUBLIC 47 8 foo"));
+ EXPECT_EQ(llvm::None, PublicRecord::parse("FUNC 47 8 foo"));
EXPECT_EQ(llvm::None, PublicRecord::parse("PUBLIC 47 8"));
EXPECT_EQ(llvm::None, PublicRecord::parse("PUBLIC 47"));
EXPECT_EQ(llvm::None, PublicRecord::parse("PUBLIC m"));
Index: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
===================================================================
--- source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
+++ source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
@@ -27,8 +27,9 @@
class LineIterator {
public:
// begin iterator for sections of given type
- LineIterator(ObjectFile &obj, ConstString section_type)
- : m_obj(&obj), m_section_type(section_type), m_next_section_idx(0) {
+ LineIterator(ObjectFile &obj, Record::Kind section_type)
+ : m_obj(&obj), m_section_type(toString(section_type)),
+ m_next_section_idx(0) {
++*this;
}
@@ -78,7 +79,7 @@
}
static llvm::iterator_range<LineIterator> lines(ObjectFile &obj,
- ConstString section_type) {
+ Record::Kind section_type) {
return llvm::make_range(LineIterator(obj, section_type), LineIterator(obj));
}
@@ -181,35 +182,40 @@
}
const SectionList &list = *module.GetSectionList();
- for (llvm::StringRef line : lines(*m_obj_file, ConstString("PUBLIC"))) {
- auto record = PublicRecord::parse(line);
- if (!record) {
- LLDB_LOG(log, "Failed to parse: {0}. Skipping record.", line);
- continue;
- }
- addr_t file_address = base + record->getAddress();
-
- SectionSP section_sp = list.FindSectionContainingFileAddress(file_address);
+ llvm::DenseMap<addr_t, Symbol> symbols;
+ auto add_symbol = [&](addr_t address, llvm::Optional<addr_t> size,
+ llvm::StringRef name) {
+ address += base;
+ SectionSP section_sp = list.FindSectionContainingFileAddress(address);
if (!section_sp) {
LLDB_LOG(log,
"Ignoring symbol {0}, whose address ({1}) is outside of the "
"object file. Mismatched symbol file?",
- record->getName(), file_address);
- continue;
+ name, address);
+ return;
}
-
- symtab.AddSymbol(Symbol(
- /*symID*/ 0, Mangled(record->getName(), /*is_mangled*/ false),
- eSymbolTypeCode,
- /*is_global*/ true, /*is_debug*/ false, /*is_trampoline*/ false,
- /*is_artificial*/ false,
- AddressRange(section_sp, file_address - section_sp->GetFileAddress(),
- 0),
- /*size_is_valid*/ 0, /*contains_linker_annotations*/ false,
- /*flags*/ 0));
+ symbols.try_emplace(
+ address, /*symID*/ 0, Mangled(name, /*is_mangled*/ false),
+ eSymbolTypeCode, /*is_global*/ true, /*is_debug*/ false,
+ /*is_trampoline*/ false, /*is_artificial*/ false,
+ AddressRange(section_sp, address - section_sp->GetFileAddress(),
+ size.getValueOr(0)),
+ size.hasValue(), /*contains_linker_annotations*/ false, /*flags*/ 0);
+ };
+
+ for (llvm::StringRef line : lines(*m_obj_file, Record::Func)) {
+ if (auto record = FuncRecord::parse(line))
+ add_symbol(record->getAddress(), record->getSize(), record->getName());
}
- // TODO: Process FUNC records as well.
+ for (llvm::StringRef line : lines(*m_obj_file, Record::Public)) {
+ if (auto record = PublicRecord::parse(line))
+ add_symbol(record->getAddress(), llvm::None, record->getName());
+ else
+ LLDB_LOG(log, "Failed to parse: {0}. Skipping record.", line);
+ }
+ for (auto &KV : symbols)
+ symtab.AddSymbol(std::move(KV.second));
symtab.CalculateSymbolSizes();
}
Index: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
===================================================================
--- source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
+++ source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
@@ -81,6 +81,31 @@
}
llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const InfoRecord &R);
+class FuncRecord : public Record {
+public:
+ static llvm::Optional<FuncRecord> parse(llvm::StringRef Line);
+ FuncRecord(bool Multiple, lldb::addr_t Address, lldb::addr_t Size,
+ lldb::addr_t ParamSize, llvm::StringRef Name)
+ : Record(Module), Multiple(Multiple), Address(Address), Size(Size),
+ ParamSize(ParamSize), Name(Name) {}
+
+ bool getMultiple() const { return Multiple; }
+ lldb::addr_t getAddress() const { return Address; }
+ lldb::addr_t getSize() const { return Size; }
+ lldb::addr_t getParamSize() const { return ParamSize; }
+ llvm::StringRef getName() const { return Name; }
+
+private:
+ bool Multiple;
+ lldb::addr_t Address;
+ lldb::addr_t Size;
+ lldb::addr_t ParamSize;
+ llvm::StringRef Name;
+};
+
+bool operator==(const FuncRecord &L, const FuncRecord &R);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const FuncRecord &R);
+
class PublicRecord : public Record {
public:
static llvm::Optional<PublicRecord> parse(llvm::StringRef Line);
Index: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
===================================================================
--- source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
+++ source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
@@ -192,32 +192,77 @@
return OS << "INFO CODE_ID " << R.getID().GetAsString();
}
-llvm::Optional<PublicRecord> PublicRecord::parse(llvm::StringRef Line) {
+static bool parsePublicOrFunc(llvm::StringRef Line, bool &Multiple,
+ lldb::addr_t &Address, lldb::addr_t *Size,
+ lldb::addr_t &ParamSize, llvm::StringRef &Name) {
// PUBLIC [m] address param_size name
+ // or
+ // FUNC [m] address size param_size name
+
+ Token Tok = Size ? Token::Func : Token::Public;
+
llvm::StringRef Str;
std::tie(Str, Line) = getToken(Line);
- if (toToken(Str) != Token::Public)
- return llvm::None;
+ if (toToken(Str) != Tok)
+ return false;
std::tie(Str, Line) = getToken(Line);
- bool Multiple = Str == "m";
+ Multiple = Str == "m";
if (Multiple)
std::tie(Str, Line) = getToken(Line);
- lldb::addr_t Address;
if (!to_integer(Str, Address, 16))
- return llvm::None;
+ return false;
+
+ if (Tok == Token::Func) {
+ std::tie(Str, Line) = getToken(Line);
+ if (!to_integer(Str, *Size, 16))
+ return false;
+ }
std::tie(Str, Line) = getToken(Line);
- lldb::addr_t ParamSize;
if (!to_integer(Str, ParamSize, 16))
- return llvm::None;
+ return false;
- llvm::StringRef Name = Line.trim();
+ Name = Line.trim();
if (Name.empty())
- return llvm::None;
+ return false;
+
+ return true;
+}
+
+llvm::Optional<FuncRecord> FuncRecord::parse(llvm::StringRef Line) {
+ bool Multiple;
+ lldb::addr_t Address, Size, ParamSize;
+ llvm::StringRef Name;
+
+ if (parsePublicOrFunc(Line, Multiple, Address, &Size, ParamSize, Name))
+ return FuncRecord(Multiple, Address, Size, ParamSize, Name);
+
+ return llvm::None;
+}
+
+bool breakpad::operator==(const FuncRecord &L, const FuncRecord &R) {
+ return L.getMultiple() == R.getMultiple() &&
+ L.getAddress() == R.getAddress() && L.getSize() == R.getSize() &&
+ L.getParamSize() == R.getParamSize() && L.getName() == R.getName();
+}
+llvm::raw_ostream &breakpad::operator<<(llvm::raw_ostream &OS,
+ const FuncRecord &R) {
+ return OS << llvm::formatv("FUNC {0}{1:x-} {2:x-} {3:x-} {4}",
+ R.getMultiple() ? "m " : "", R.getAddress(),
+ R.getSize(), R.getParamSize(), R.getName());
+}
+
+llvm::Optional<PublicRecord> PublicRecord::parse(llvm::StringRef Line) {
+ bool Multiple;
+ lldb::addr_t Address, ParamSize;
+ llvm::StringRef Name;
+
+ if (parsePublicOrFunc(Line, Multiple, Address, nullptr, ParamSize, Name))
+ return PublicRecord(Multiple, Address, ParamSize, Name);
- return PublicRecord(Multiple, Address, ParamSize, Name);
+ return llvm::None;
}
bool breakpad::operator==(const PublicRecord &L, const PublicRecord &R) {
Index: lit/SymbolFile/Breakpad/symtab.test
===================================================================
--- lit/SymbolFile/Breakpad/symtab.test
+++ lit/SymbolFile/Breakpad/symtab.test
@@ -3,15 +3,16 @@
# RUN: -s %s | FileCheck %s
# CHECK-LABEL: (lldb) image dump symtab symtab.out
-# CHECK: Symtab, file = {{.*}}symtab.out, num_symbols = 3:
+# CHECK: Symtab, file = {{.*}}symtab.out, num_symbols = 4:
# CHECK: Index UserID DSX Type File Address/Value Load Address Size Flags Name
-# CHECK: [ 0] 0 X Code 0x00000000004000b0 0x0000000000000010 0x00000000 f1
-# CHECK: [ 1] 0 X Code 0x00000000004000c0 0x0000000000000010 0x00000000 f2
-# CHECK: [ 2] 0 X Code 0x00000000004000d0 0x0000000000000022 0x00000000 _start
+# CHECK: [ 0] 0 X Code 0x00000000004000c0 0x0000000000000010 0x00000000 f2
+# CHECK: [ 1] 0 X Code 0x00000000004000d0 0x0000000000000022 0x00000000 _start
+# CHECK: [ 2] 0 X Code 0x00000000004000a0 0x000000000000000d 0x00000000 func_only
+# CHECK: [ 3] 0 X Code 0x00000000004000b0 0x000000000000000c 0x00000000 f1_func
# CHECK-LABEL: (lldb) image lookup -a 0x4000b0 -v
# CHECK: Address: symtab.out[0x00000000004000b0] (symtab.out.PT_LOAD[0]..text2 + 0)
-# CHECK: Symbol: id = {0x00000000}, range = [0x00000000004000b0-0x00000000004000c0), name="f1"
+# CHECK: Symbol: id = {0x00000000}, range = [0x00000000004000b0-0x00000000004000bc), name="f1_func"
# CHECK-LABEL: (lldb) image lookup -n f2 -v
# CHECK: Address: symtab.out[0x00000000004000c0] (symtab.out.PT_LOAD[0]..text2 + 16)
Index: lit/SymbolFile/Breakpad/Inputs/symtab.syms
===================================================================
--- lit/SymbolFile/Breakpad/Inputs/symtab.syms
+++ lit/SymbolFile/Breakpad/Inputs/symtab.syms
@@ -5,3 +5,5 @@
PUBLIC m c0 0 f2
PUBLIC d0 0 _start
PUBLIC ff 0 _out_of_range_ignored
+FUNC b0 c 0 f1_func
+FUNC m a0 d 0 func_only
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
