Skip to content

Commit a881160

Browse files
r1violletr1viollet
committed
Inlining - refactoring
Ensuring the line map has a well defined type
1 parent 3ddddd2 commit a881160

7 files changed

Lines changed: 69 additions & 68 deletions

File tree

‎include/dwfl_symbol_lookup.hpp‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@ class DwflSymbolLookup {
6464
DwflSymbolLookupStats &stats() { return _stats; }
6565

6666
// todo: we can have a better type than symbol idx for the line
67-
using LineMap = SymbolMap;
6867
using InlineMap = NestedSymbolMap;
6968
struct SymbolWrapper {
7069
LineMap _line_map;

‎include/symbol_map.hpp‎

Lines changed: 48 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,10 @@
1111

1212
namespace ddprof {
1313

14-
// template<typename T>
15-
class SymbolSpan {
14+
template <typename T, T DefaultValue = T()> class TSpan {
1615
public:
17-
SymbolSpan() : _end(0), _symbol_idx(-1) {}
18-
SymbolSpan(Offset_t end, SymbolIdx_t symbol_idx)
19-
: _end(end), _symbol_idx(symbol_idx) {}
16+
TSpan() : _end(0), _value(DefaultValue) {}
17+
TSpan(Offset_t end, T value) : _end(end), _value(value) {}
2018
// push end further
2119
void set_end(Offset_t end) {
2220
if (end > _end) {
@@ -25,23 +23,27 @@ class SymbolSpan {
2523
}
2624
[[nodiscard]] Offset_t get_end() const { return _end; }
2725

28-
[[nodiscard]] SymbolIdx_t get_symbol_idx() const { return _symbol_idx; }
26+
[[nodiscard]] T get_value() const { return _value; }
2927

3028
private:
3129
// symbol end within the segment (considering file offset)
3230
Offset_t _end;
3331
// element inside internal symbol cache
34-
SymbolIdx_t _symbol_idx;
32+
T _value;
3533
};
3634

37-
class SymbolMap : private std::map<ElfAddress_t, SymbolSpan> {
35+
using SymbolSpan = TSpan<SymbolIdx_t, -1>;
36+
using LineSpan = TSpan<uint32_t, 0>;
37+
38+
template <typename TSpan>
39+
class SpanMap : private std::map<ElfAddress_t, TSpan> {
3840
public:
39-
using Map = std::map<ElfAddress_t, SymbolSpan>;
40-
using It = Map::iterator;
41-
using ConstIt = Map::const_iterator;
41+
using Map = std::map<ElfAddress_t, TSpan>;
42+
using It = typename Map::iterator;
43+
using ConstIt = typename Map::const_iterator;
4244
using FindRes = std::pair<It, bool>;
4345
using ValueType =
44-
Map::value_type; // key value pair ElfAddress_t, SymbolSpanMap
46+
typename Map::value_type; // key value pair ElfAddress_t, SymbolSpanMap
4547
// Functions we forward from underlying map type
4648

4749
using Map::begin;
@@ -53,10 +55,39 @@ class SymbolMap : private std::map<ElfAddress_t, SymbolSpan> {
5355
using Map::erase;
5456
using Map::size;
5557

56-
static bool is_within(const Offset_t &norm_pc, const ValueType &kv);
57-
FindRes find_closest(Offset_t norm_pc);
58+
bool is_within(const Offset_t &norm_pc, const SpanMap::ValueType &kv) {
59+
if (norm_pc < kv.first) {
60+
return false;
61+
}
62+
if (norm_pc > kv.second.get_end()) {
63+
return false;
64+
}
65+
return true;
66+
}
67+
68+
FindRes find_closest(Offset_t norm_pc) {
69+
// First element not less than (can match exactly a start addr)
70+
auto it = Map::lower_bound(norm_pc);
71+
if (it != end()) { // map is empty
72+
if (SpanMap::is_within(norm_pc, *it)) {
73+
return {it, true};
74+
}
75+
}
76+
77+
// previous element is more likely to contain our addr
78+
if (it != begin()) {
79+
--it;
80+
} else { // map is empty
81+
return {end(), false};
82+
}
83+
// element can not be end (as we reversed or exit)
84+
return {it, is_within(norm_pc, *it)};
85+
}
5886
};
5987

88+
using SymbolMap = SpanMap<SymbolSpan>;
89+
using LineMap = SpanMap<LineSpan>;
90+
6091
class NestedSymbolValue {
6192
public:
6293
NestedSymbolValue() : _symbol_idx(-1), _call_line_number(0) {}
@@ -99,9 +130,12 @@ class NestedSymbolMap : private std::map<NestedSymbolKey, NestedSymbolValue> {
99130
using Map::erase;
100131
using Map::size;
101132

133+
// todo: possible improvement to return a table of all elements matching
134+
102135
FindRes find_parent(ConstIt it, const NestedSymbolKey &parent_bound,
103136
Offset_t norm_pc) const;
104137

138+
// returns the element that is the most leaf
105139
FindRes find_closest(Offset_t norm_pc,
106140
const NestedSymbolKey &parent_bound) const;
107141

‎src/ddprof_cli.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ int DDProfCLI::parse(int argc, const char *argv[]) {
163163
->group("Profiling settings")
164164
->excludes(pid_opt)
165165
->excludes(exec_option);
166-
166+
167167
app.add_option("--inlining", inlining, "Add inlining information.\n")
168168
->group("Profiling settings")
169169
->default_val(true);

‎src/dwfl_symbol_lookup.cc‎

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -86,14 +86,13 @@ void DwflSymbolLookup::add_fun_loc(
8686
// line can be associated to parent
8787
const auto line_find = symbol_wrapper._line_map.find_closest(elf_pc);
8888
if (line_find.second) {
89-
line = line_find.first->second.get_symbol_idx();
89+
line = line_find.first->second.get_value();
9090
}
9191
}
92-
func_locs.emplace_back(
93-
FunLoc{._ip = process_pc,
94-
._lineno = line,
95-
._symbol_idx = parent_sym.second.get_symbol_idx(),
96-
._map_info_idx = -1});
92+
func_locs.emplace_back(FunLoc{._ip = process_pc,
93+
._lineno = line,
94+
._symbol_idx = parent_sym.second.get_value(),
95+
._map_info_idx = -1});
9796
}
9897

9998
// Retrieve existing symbol or attempt to read from dwarf
@@ -117,14 +116,14 @@ void DwflSymbolLookup::get_or_insert(Dwfl *dwfl, const DDProfMod &ddprof_mod,
117116
#ifdef DEBUG
118117
LG_DBG("Match: %lx,%lx -> %s,%d", find_res.first->first,
119118
find_res.first->second.get_end(),
120-
table[find_res.first->second.get_symbol_idx()]._symname.c_str(),
121-
find_res.first->second.get_symbol_idx());
119+
table[find_res.first->second.get_value()]._symname.c_str(),
120+
find_res.first->second.get_value());
122121
#endif
123122
// cache validation mechanism: force dwfl lookup to compare with matched
124123
// symbols
125124
if (_lookup_setting == K_CACHE_VALIDATE) {
126125
if (symbol_lookup_check(ddprof_mod._mod, process_pc,
127-
table[find_res.first->second.get_symbol_idx()])) {
126+
table[find_res.first->second.get_value()])) {
128127
++_stats._errors;
129128
}
130129
}
@@ -160,7 +159,7 @@ static DDRes parse_lines(Dwarf_Die *cudie, const DDProfMod &mod,
160159
DwflSymbolLookup::SymbolWrapper &symbol_wrapper,
161160
SymbolTable &table, DieInformation &die_information) {
162161

163-
DwflSymbolLookup::LineMap &line_map = symbol_wrapper._line_map;
162+
LineMap &line_map = symbol_wrapper._line_map;
164163
DwflSymbolLookup::InlineMap &inline_map = symbol_wrapper._inline_map;
165164
Dwarf_Lines *lines;
166165
size_t nlines;
@@ -184,7 +183,7 @@ static DDRes parse_lines(Dwarf_Die *cudie, const DDProfMod &mod,
184183
Dwarf_Addr previous_addr = 0;
185184
NestedSymbolMap::FindRes current_func{inline_map.end(), false};
186185
// store closest line per file (to avoid missmatches)
187-
std::unordered_map<std::string, int> closest_lines;
186+
std::unordered_map<std::string, uint32_t> closest_lines;
188187
for (size_t line_index = start_index; line_index < nlines; ++line_index) {
189188
Dwarf_Line *line = dwarf_onesrcline(lines, line_index);
190189
Dwarf_Addr line_addr;
@@ -201,8 +200,7 @@ static DDRes parse_lines(Dwarf_Die *cudie, const DDProfMod &mod,
201200

202201
if (previous_addr && line_addr != previous_addr) {
203202
if (hint_line != line_map.end() &&
204-
hint_line->second.get_symbol_idx() ==
205-
closest_lines[ref_sym->_srcpath]) {
203+
hint_line->second.get_value() == closest_lines[ref_sym->_srcpath]) {
206204
// extend previous element
207205
hint_line->second.set_end(previous_addr);
208206
} else {
@@ -211,7 +209,7 @@ static DDRes parse_lines(Dwarf_Die *cudie, const DDProfMod &mod,
211209
hint_line,
212210
std::make_pair(
213211
previous_addr,
214-
SymbolSpan{line_addr - 1, closest_lines[ref_sym->_srcpath]}));
212+
LineSpan{line_addr - 1, closest_lines[ref_sym->_srcpath]}));
215213
}
216214
#ifdef DEBUG
217215
LG_DBG("Associate %d (%lx->%lx) / %s to %s (vs %s)",
@@ -231,7 +229,7 @@ static DDRes parse_lines(Dwarf_Die *cudie, const DDProfMod &mod,
231229
}
232230
// keep line, if it matches the symbol
233231
// todo can be optimized to avoid conversion to string
234-
closest_lines[std::string(current_file)] = lineno;
232+
closest_lines[std::string(current_file)] = static_cast<uint32_t>(lineno);
235233
previous_addr = line_addr;
236234
}
237235
return {};
@@ -242,7 +240,7 @@ DDRes DwflSymbolLookup::insert_inlining_info(
242240
ProcessAddress_t process_pc, const Dso &dso, SymbolWrapper &symbol_wrapper,
243241
SymbolMap::ValueType &parent_func) {
244242

245-
SymbolIdx_t parent_sym_idx = parent_func.second.get_symbol_idx();
243+
SymbolIdx_t parent_sym_idx = parent_func.second.get_value();
246244
Dwarf_Addr bias;
247245
Dwarf_Die *cudie = dwfl_addrdie(dwfl, process_pc, &bias);
248246
if (!cudie) {
@@ -388,7 +386,7 @@ NestedSymbolMap::FindRes DwflSymbolLookup::get_inlined(
388386
} else {
389387
auto find_line = symbol_wrapper._line_map.find_closest(elf_pc);
390388
if (find_line.second) {
391-
line = find_line.first->second.get_symbol_idx();
389+
line = find_line.first->second.get_value();
392390
}
393391
}
394392
func_locs.emplace_back(

‎src/runtime_symbol_lookup.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ bool RuntimeSymbolLookup::insert_or_replace(std::string_view symbol,
7474
"jit");
7575
} else {
7676
// todo managing range erase (we can overal with other syms)
77-
SymbolIdx_t const existing = find_res.first->second.get_symbol_idx();
77+
SymbolIdx_t const existing = find_res.first->second.get_value();
7878
#ifdef DEBUG
7979
LG_DBG("Existyng sym -- %s (%lx-%lx)",
8080
symbol_table[existing]._demangle_name.c_str(), find_res.first->first,
@@ -194,7 +194,7 @@ RuntimeSymbolLookup::get_or_insert_jitdump(pid_t pid, ProcessAddress_t pc,
194194
if (!find_res.second) {
195195
flag_lookup_failure(symbol_info, jitdump_path);
196196
}
197-
return find_res.second ? find_res.first->second.get_symbol_idx() : -1;
197+
return find_res.second ? find_res.first->second.get_value() : -1;
198198
}
199199

200200
SymbolIdx_t RuntimeSymbolLookup::get_or_insert(pid_t pid, ProcessAddress_t pc,
@@ -211,7 +211,7 @@ SymbolIdx_t RuntimeSymbolLookup::get_or_insert(pid_t pid, ProcessAddress_t pc,
211211
if (!find_res.second) {
212212
flag_lookup_failure(symbol_info, "perfmap");
213213
}
214-
return find_res.second ? find_res.first->second.get_symbol_idx() : -1;
214+
return find_res.second ? find_res.first->second.get_value() : -1;
215215
}
216216

217217
} // namespace ddprof

‎src/symbol_map.cc‎

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -9,36 +9,6 @@
99

1010
namespace ddprof {
1111

12-
bool SymbolMap::is_within(const Offset_t &norm_pc,
13-
const SymbolMap::ValueType &kv) {
14-
if (norm_pc < kv.first) {
15-
return false;
16-
}
17-
if (norm_pc > kv.second.get_end()) {
18-
return false;
19-
}
20-
return true;
21-
}
22-
23-
SymbolMap::FindRes SymbolMap::find_closest(Offset_t norm_pc) {
24-
// First element not less than (can match exactly a start addr)
25-
auto it = lower_bound(norm_pc);
26-
if (it != end()) { // map is empty
27-
if (SymbolMap::is_within(norm_pc, *it)) {
28-
return {it, true};
29-
}
30-
}
31-
32-
// previous element is more likely to contain our addr
33-
if (it != begin()) {
34-
--it;
35-
} else { // map is empty
36-
return {end(), false};
37-
}
38-
// element can not be end (as we reversed or exit)
39-
return {it, is_within(norm_pc, *it)};
40-
}
41-
4212
// parent span acts as a bound
4313
NestedSymbolMap::FindRes
4414
NestedSymbolMap::find_parent(NestedSymbolMap::ConstIt it,

‎test/symbol_map-ut.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,10 @@ namespace ddprof {
1313
TEST(SymbolMap, Span) {
1414
SymbolSpan span1;
1515
EXPECT_EQ(span1.get_end(), 0);
16-
EXPECT_EQ(span1.get_symbol_idx(), -1);
16+
EXPECT_EQ(span1.get_value(), -1);
1717
SymbolSpan span2(0x1000, 12);
1818
EXPECT_EQ(span2.get_end(), 0x1000);
19-
EXPECT_EQ(span2.get_symbol_idx(), 12);
19+
EXPECT_EQ(span2.get_value(), 12);
2020
}
2121

2222
TEST(SymbolMap, Map) {

0 commit comments

Comments
 (0)