Re: [Lldb-commits] [lldb] cd64273 - [lldb/ELF] Fix IDs of synthetic eh_frame symbols
On 21/04/2021 22:13, Shafik Yaghmour wrote: Having side effects in the argument to a function call just feels like a bad idea in general even if it seems obviously correct here e.g. ++last_symbol_id, Can’t we just do: uint64_t last_symbol_id = num_symbols ? symbol_table->SymbolAtIndex(num_symbols - 1)->GetID() + 1: 1; This would just change the pre-increment to a post-increment. The increment still needs to happen somewhere, as the function can add more than one symbol. However, I don't have a problem with moving the increment out of the function call. That is what I've done in e5984a3. Thanks for reviewing this, Pavel ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] cd64273 - [lldb/ELF] Fix IDs of synthetic eh_frame symbols
Having side effects in the argument to a function call just feels like a bad
idea in general even if it seems obviously correct here e.g.
++last_symbol_id,
Can’t we just do:
uint64_t last_symbol_id =
num_symbols ? symbol_table->SymbolAtIndex(num_symbols - 1)->GetID() + 1:
1;
Thank you
> On Apr 21, 2021, at 2:25 AM, Pavel Labath via lldb-commits
> wrote:
>
>
> Author: Pavel Labath
> Date: 2021-04-21T11:24:43+02:00
> New Revision: cd64273f5ed39ec697ff1e20a1fe25ebd3502629
>
> URL:
> https://github.com/llvm/llvm-project/commit/cd64273f5ed39ec697ff1e20a1fe25ebd3502629
> DIFF:
> https://github.com/llvm/llvm-project/commit/cd64273f5ed39ec697ff1e20a1fe25ebd3502629.diff
>
> LOG: [lldb/ELF] Fix IDs of synthetic eh_frame symbols
>
> The code used the total number of symbols to create a symbol ID for the
> synthetic symbols. This is not correct because the IDs of real symbols
> can be higher than their total number, as we do not add all symbols (and
> in particular, we never add symbol zero, which is not a real symbol).
>
> This meant we could have symbols with duplicate IDs, which caused
> problems if some relocations were referring to the duplicated IDs. This
> was the cause of the failure of the test D97786.
>
> This patch fixes the code to use the ID of the highest (last) symbol
> instead.
>
> Added:
>lldb/test/Shell/ObjectFile/ELF/eh_frame-symbols.yaml
>
> Modified:
>lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
>
> Removed:
>
>
>
>
> diff --git a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
> b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
> index f30ed427f8535..6e94ab97992a1 100644
> --- a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
> +++ b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
> @@ -2901,8 +2901,11 @@ void ObjectFileELF::ParseUnwindSymbols(Symtab
> *symbol_table,
> // recalculate the index first.
> std::vector new_symbols;
>
> - eh_frame->ForEachFDEEntries([this, symbol_table, section_list,
> _symbols](
> - lldb::addr_t file_addr, uint32_t size, dw_offset_t) {
> + size_t num_symbols = symbol_table->GetNumSymbols();
> + uint64_t last_symbol_id =
> + num_symbols ? symbol_table->SymbolAtIndex(num_symbols - 1)->GetID() :
> 0;
> + eh_frame->ForEachFDEEntries([&](lldb::addr_t file_addr, uint32_t size,
> + dw_offset_t) {
> Symbol *symbol = symbol_table->FindSymbolAtFileAddress(file_addr);
> if (symbol) {
> if (!symbol->GetByteSizeIsValid()) {
> @@ -2915,21 +2918,20 @@ void ObjectFileELF::ParseUnwindSymbols(Symtab
> *symbol_table,
> if (section_sp) {
> addr_t offset = file_addr - section_sp->GetFileAddress();
> const char *symbol_name = GetNextSyntheticSymbolName().GetCString();
> -uint64_t symbol_id = symbol_table->GetNumSymbols();
> Symbol eh_symbol(
> -symbol_id, // Symbol table index.
> -symbol_name, // Symbol name.
> -eSymbolTypeCode, // Type of this symbol.
> -true,// Is this globally visible?
> -false, // Is this symbol debug info?
> -false, // Is this symbol a trampoline?
> -true,// Is this symbol artificial?
> -section_sp, // Section in which this symbol is defined or
> null.
> -offset, // Offset in section or symbol value.
> -0, // Size: Don't specify the size as an FDE can
> -false, // Size is valid: cover multiple symbols.
> -false, // Contains linker annotations?
> -0);// Symbol flags.
> +++last_symbol_id, // Symbol table index.
> +symbol_name, // Symbol name.
> +eSymbolTypeCode, // Type of this symbol.
> +true, // Is this globally visible?
> +false,// Is this symbol debug info?
> +false,// Is this symbol a trampoline?
> +true, // Is this symbol artificial?
> +section_sp, // Section in which this symbol is defined or null.
> +offset, // Offset in section or symbol value.
> +0, // Size: Don't specify the size as an FDE
> can
> +false, // Size is valid: cover multiple symbols.
> +false, // Contains linker annotations?
> +0); // Symbol flags.
> new_symbols.push_back(eh_symbol);
> }
> }
>
> diff --git a/lldb/test/Shell/ObjectFile/ELF/eh_frame-symbols.yaml
> b/lldb/test/Shell/ObjectFile/ELF/eh_frame-symbols.yaml
> new file mode 100644
> index 0..6178a45de1b59
> --- /dev/null
> +++ b/lldb/test/Shell/ObjectFile/ELF/eh_frame-symbols.yaml
> @@ -0,0 +1,32 @@
> +# RUN: yaml2obj %s >%t
> +# RUN: %lldb %t -o "image dump
[Lldb-commits] [lldb] cd64273 - [lldb/ELF] Fix IDs of synthetic eh_frame symbols
Author: Pavel Labath
Date: 2021-04-21T11:24:43+02:00
New Revision: cd64273f5ed39ec697ff1e20a1fe25ebd3502629
URL:
https://github.com/llvm/llvm-project/commit/cd64273f5ed39ec697ff1e20a1fe25ebd3502629
DIFF:
https://github.com/llvm/llvm-project/commit/cd64273f5ed39ec697ff1e20a1fe25ebd3502629.diff
LOG: [lldb/ELF] Fix IDs of synthetic eh_frame symbols
The code used the total number of symbols to create a symbol ID for the
synthetic symbols. This is not correct because the IDs of real symbols
can be higher than their total number, as we do not add all symbols (and
in particular, we never add symbol zero, which is not a real symbol).
This meant we could have symbols with duplicate IDs, which caused
problems if some relocations were referring to the duplicated IDs. This
was the cause of the failure of the test D97786.
This patch fixes the code to use the ID of the highest (last) symbol
instead.
Added:
lldb/test/Shell/ObjectFile/ELF/eh_frame-symbols.yaml
Modified:
lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
Removed:
diff --git a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
index f30ed427f8535..6e94ab97992a1 100644
--- a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -2901,8 +2901,11 @@ void ObjectFileELF::ParseUnwindSymbols(Symtab
*symbol_table,
// recalculate the index first.
std::vector new_symbols;
- eh_frame->ForEachFDEEntries([this, symbol_table, section_list, _symbols](
- lldb::addr_t file_addr, uint32_t size, dw_offset_t) {
+ size_t num_symbols = symbol_table->GetNumSymbols();
+ uint64_t last_symbol_id =
+ num_symbols ? symbol_table->SymbolAtIndex(num_symbols - 1)->GetID() : 0;
+ eh_frame->ForEachFDEEntries([&](lldb::addr_t file_addr, uint32_t size,
+ dw_offset_t) {
Symbol *symbol = symbol_table->FindSymbolAtFileAddress(file_addr);
if (symbol) {
if (!symbol->GetByteSizeIsValid()) {
@@ -2915,21 +2918,20 @@ void ObjectFileELF::ParseUnwindSymbols(Symtab
*symbol_table,
if (section_sp) {
addr_t offset = file_addr - section_sp->GetFileAddress();
const char *symbol_name = GetNextSyntheticSymbolName().GetCString();
-uint64_t symbol_id = symbol_table->GetNumSymbols();
Symbol eh_symbol(
-symbol_id, // Symbol table index.
-symbol_name, // Symbol name.
-eSymbolTypeCode, // Type of this symbol.
-true,// Is this globally visible?
-false, // Is this symbol debug info?
-false, // Is this symbol a trampoline?
-true,// Is this symbol artificial?
-section_sp, // Section in which this symbol is defined or
null.
-offset, // Offset in section or symbol value.
-0, // Size: Don't specify the size as an FDE can
-false, // Size is valid: cover multiple symbols.
-false, // Contains linker annotations?
-0);// Symbol flags.
+++last_symbol_id, // Symbol table index.
+symbol_name, // Symbol name.
+eSymbolTypeCode, // Type of this symbol.
+true, // Is this globally visible?
+false,// Is this symbol debug info?
+false,// Is this symbol a trampoline?
+true, // Is this symbol artificial?
+section_sp, // Section in which this symbol is defined or null.
+offset, // Offset in section or symbol value.
+0, // Size: Don't specify the size as an FDE can
+false, // Size is valid: cover multiple symbols.
+false, // Contains linker annotations?
+0); // Symbol flags.
new_symbols.push_back(eh_symbol);
}
}
diff --git a/lldb/test/Shell/ObjectFile/ELF/eh_frame-symbols.yaml
b/lldb/test/Shell/ObjectFile/ELF/eh_frame-symbols.yaml
new file mode 100644
index 0..6178a45de1b59
--- /dev/null
+++ b/lldb/test/Shell/ObjectFile/ELF/eh_frame-symbols.yaml
@@ -0,0 +1,32 @@
+# RUN: yaml2obj %s >%t
+# RUN: %lldb %t -o "image dump symtab" -b | FileCheck %s
+
+# CHECK: Index UserID DSX TypeFile Address/Value Load Address
Size Flags Name
+# CHECK: [0] 1 SourceFile 0x
0x 0x0004 -
+# CHECK: [1] 2 SX Code0x00201180
0x0010 0x ___lldb_unnamed_symbol1$${{.*}}
+# CHECK: [2] 3 SX Code0x00201190
0x0006 0x ___lldb_unnamed_symbol2$${{.*}}
+
+--- !ELF
