[Lldb-commits] [lldb] r258113 - Unconditionally accept symbol sizes from elf
Adrian McCarthy via lldb-commits
lldb-commits at lists.llvm.org
Wed Jan 20 15:19:37 PST 2016
In `Symtab::FindSymbolContainingFileAddress`, the address range for the
symbols we're finding always have `m_byte_size == 0`, so the extra check
`symbol->ContainsFileAddress(file_addr)` always fails, causing us to return
nullptr instead of the symbol.
I don't know if that's the right behavior, but that is the change that's
causing thread `step-over` to fail on Windows, which takes out about a
dozen tests.
On Wed, Jan 20, 2016 at 2:33 PM, Zachary Turner via lldb-commits <
lldb-commits at lists.llvm.org> wrote:
> Hi Tamas,
>
> I think this build breaks Windows. This is actually the breakage that we
> were discussing in the thread about thread step-over.
>
> 1 revision before your patch:
> D:\src\llvmbuild\ninja>C:\Python35\python_d.exe
> D:/src/llvm/tools/lldb/test/dotest.py -q --arch=i686 --executable
> D:/src/llvmbuild/ninja/bin/lldb.exe -s
> D:/src/llvmbuild/ninja/lldb-test-traces -u CXXFLAGS -u CFLAGS
> --enable-crash-dialog -C d:\src\llvmbuild\ninja_release\bin\clang.exe -p
> TestSignedTypes.py D:\src\llvm\tools\lldb\packages\Python\lldbsuite\test
> --no-multiprocess
> LLDB library dir: D:/src/llvmbuild/ninja/bin
> LLDB import library dir: D:/src/llvmbuild/ninja/bin\..\lib
> lldb version 3.9.0 clang revision 257802 llvm revision 257804
> The 'lldb-mi' executable cannot be located. The lldb-mi tests can not be
> run as a result.
>
> Session logs for test failures/errors/unexpected successes will go into
> directory 'D:/src/llvmbuild/ninja/lldb-test-traces'
> Command invoked: D:/src/llvm/tools/lldb/test/dotest.py -q --arch=i686
> --executable D:/src/llvmbuild/ninja/bin/lldb.exe -s
> D:/src/llvmbuild/ninja/lldb-test-traces -u CXXFLAGS -u CFLAGS
> --enable-crash-dialog -C d:\src\llvmbuild\ninja_release\bin\clang.exe -p
> TestSignedTypes.py D:\src\llvm\tools\lldb\packages\Python\lldbsuite\test
> --no-multiprocess
> PASS: LLDB (d:\src\llvmbuild\ninja_release\bin\clang.exe-i686) ::
> test_dwarf (TestSignedTypes.UnsignedTypesTestCase)
> ----------------------------------------------------------------------
> Ran 1 test in 1.820s
>
> RESULT: PASSED (1 passes, 0 failures, 0 errors, 0 skipped, 0 expected
> failures, 0 unexpected successes)
>
> With your patch:
> D:\src\llvmbuild\ninja>C:\Python35\python_d.exe
> D:/src/llvm/tools/lldb/test/dotest.py -q --arch=i686 --executable
> D:/src/llvmbuild/ninja/bin/lldb.exe -s
> D:/src/llvmbuild/ninja/lldb-test-traces -u CXXFLAGS -u CFLAGS
> --enable-crash-dialog -C d:\src\llvmbuild\ninja_release\bin\clang.exe -p
> TestSignedTypes.py D:\src\llvm\tools\lldb\packages\Python\lldbsuite\test
> --no-multiprocess
> LLDB library dir: D:/src/llvmbuild/ninja/bin
> LLDB import library dir: D:/src/llvmbuild/ninja/bin\..\lib
> lldb version 3.9.0 clang revision 257802 llvm revision 257804
> The 'lldb-mi' executable cannot be located. The lldb-mi tests can not be
> run as a result.
>
> Session logs for test failures/errors/unexpected successes will go into
> directory 'D:/src/llvmbuild/ninja/lldb-test-traces'
> Command invoked: D:/src/llvm/tools/lldb/test/dotest.py -q --arch=i686
> --executable D:/src/llvmbuild/ninja/bin/lldb.exe -s
> D:/src/llvmbuild/ninja/lldb-test-traces -u CXXFLAGS -u CFLAGS
> --enable-crash-dialog -C d:\src\llvmbuild\ninja_release\bin\clang.exe -p
> TestSignedTypes.py D:\src\llvm\tools\lldb\packages\Python\lldbsuite\test
> --no-multiprocess
> FAIL: LLDB (d:\src\llvmbuild\ninja_release\bin\clang.exe-i686) ::
> test_dwarf (TestSignedTypes.UnsignedTypesTestCase)
> ======================================================================
> FAIL: test_dwarf (TestSignedTypes.UnsignedTypesTestCase)
> Test that variables with signed types display correctly.
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File
> "D:\src\llvm\tools\lldb\packages\Python\lldbsuite\test\lldbtest.py", line
> 2249, in dwarf_test_method
> return attrvalue(self)
> File
> "D:\src\llvm\tools\lldb\packages\Python\lldbsuite\test\lang\cpp\signed_types\TestSignedTypes.py",
> line 60, in test
> "(long long) the_signed_long_long = 99"])
> File
> "D:\src\llvm\tools\lldb\packages\Python\lldbsuite\test\lldbtest.py", line
> 2701, in expect
> msg if msg else EXP_MSG(str, exe))
> AssertionError: False is not True : Variable(s) displayed correctly
> Config=i686-d:\src\llvmbuild\ninja_release\bin\clang.exe
> ----------------------------------------------------------------------
> Ran 1 test in 1.641s
>
> RESULT: FAILED (0 passes, 1 failures, 0 errors, 0 skipped, 0 expected
> failures, 0 unexpected successes)
>
>
> We're going to look into whether the test was *actually* working before or
> if it was a false positive (i.e. it actually should have failed), but is
> there anything you can do on your end to figure out if there's other
> problems with this patch? Do you guys have windows machines?
>
> On Tue, Jan 19, 2016 at 2:28 AM Tamas Berghammer via lldb-commits <
> lldb-commits at lists.llvm.org> wrote:
>
>> Author: tberghammer
>> Date: Tue Jan 19 04:24:51 2016
>> New Revision: 258113
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=258113&view=rev
>> Log:
>> Unconditionally accept symbol sizes from elf
>>
>> The ELF symbol table always contain the size of the symbols so we
>> don't have to try to guess them based on the address of the next
>> symbol (it is needed for mach-o).
>>
>> The change fixes an issue when a symbol is removed after a 0 size
>> symbol (e.g. because the second one is not public) what previously
>> caused the symbol lookup algorithm to end up with showing the 0 size
>> symbol even for the later addresses (what are not part of any symbol).
>> That symbol lookup error can confuse the user and also confuses the
>> current stack unwinder.
>>
>> Re-commit this CL after fixing the issue with gcc-4.9.2 on i386 Linux.
>>
>> Differential revision: http://reviews.llvm.org/D16186
>>
>> Modified:
>> lldb/trunk/include/lldb/Symbol/Symbol.h
>> lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
>> lldb/trunk/source/Symbol/Symbol.cpp
>> lldb/trunk/source/Symbol/Symtab.cpp
>>
>> Modified: lldb/trunk/include/lldb/Symbol/Symbol.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Symbol/Symbol.h?rev=258113&r1=258112&r2=258113&view=diff
>>
>> ==============================================================================
>> --- lldb/trunk/include/lldb/Symbol/Symbol.h (original)
>> +++ lldb/trunk/include/lldb/Symbol/Symbol.h Tue Jan 19 04:24:51 2016
>> @@ -383,6 +383,9 @@ public:
>> bool prefer_file_cache,
>> Stream &strm);
>>
>> + bool
>> + ContainsFileAddress (lldb::addr_t file_addr) const;
>> +
>> protected:
>> // This is the internal guts of ResolveReExportedSymbol, it assumes
>> reexport_name is not null, and that module_spec
>> // is valid. We track the modules we've already seen to make sure
>> we don't get caught in a cycle.
>>
>> Modified: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp?rev=258113&r1=258112&r2=258113&view=diff
>>
>> ==============================================================================
>> --- lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp (original)
>> +++ lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp Tue Jan 19
>> 04:24:51 2016
>> @@ -2283,6 +2283,12 @@ ObjectFileELF::ParseSymbols (Symtab *sym
>> mangled.SetDemangledName( ConstString((demangled_name +
>> suffix).str()) );
>> }
>>
>> + // In ELF all symbol should have a valid size but it is not true
>> for some code symbols
>> + // coming from hand written assembly. As none of the code symbol
>> should have 0 size we try
>> + // to calculate the size for these symbols in the symtab with
>> saying that their original
>> + // size is not valid.
>> + bool symbol_size_valid = symbol.st_size != 0 || symbol_type !=
>> eSymbolTypeCode;
>> +
>> Symbol dc_symbol(
>> i + start_id, // ID is the original symbol table index.
>> mangled,
>> @@ -2295,7 +2301,7 @@ ObjectFileELF::ParseSymbols (Symtab *sym
>> symbol_section_sp, // Section in which this symbol is
>> defined or null.
>> symbol_value, // Offset in section or symbol value.
>> symbol.st_size), // Size in bytes of this symbol.
>> - symbol.st_size != 0, // Size is valid if it is not 0
>> + symbol_size_valid, // Symbol size is valid
>> has_suffix, // Contains linker annotations?
>> flags); // Symbol flags.
>> symtab->AddSymbol(dc_symbol);
>> @@ -2304,7 +2310,9 @@ ObjectFileELF::ParseSymbols (Symtab *sym
>> }
>>
>> unsigned
>> -ObjectFileELF::ParseSymbolTable(Symtab *symbol_table, user_id_t
>> start_id, lldb_private::Section *symtab)
>> +ObjectFileELF::ParseSymbolTable(Symtab *symbol_table,
>> + user_id_t start_id,
>> + lldb_private::Section *symtab)
>> {
>> if (symtab->GetObjectFile() != this)
>> {
>>
>> Modified: lldb/trunk/source/Symbol/Symbol.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Symbol/Symbol.cpp?rev=258113&r1=258112&r2=258113&view=diff
>>
>> ==============================================================================
>> --- lldb/trunk/source/Symbol/Symbol.cpp (original)
>> +++ lldb/trunk/source/Symbol/Symbol.cpp Tue Jan 19 04:24:51 2016
>> @@ -737,3 +737,10 @@ Symbol::GetDisassembly (const ExecutionC
>> }
>> return false;
>> }
>> +
>> +bool
>> +Symbol::ContainsFileAddress (lldb::addr_t file_addr) const
>> +{
>> + return m_addr_range.ContainsFileAddress(file_addr);
>> +}
>> +
>>
>> Modified: lldb/trunk/source/Symbol/Symtab.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Symbol/Symtab.cpp?rev=258113&r1=258112&r2=258113&view=diff
>>
>> ==============================================================================
>> --- lldb/trunk/source/Symbol/Symtab.cpp (original)
>> +++ lldb/trunk/source/Symbol/Symtab.cpp Tue Jan 19 04:24:51 2016
>> @@ -971,9 +971,11 @@ Symtab::InitAddressIndexes()
>> if (end_section_file_addr > symbol_file_addr)
>> {
>> Symbol &symbol = m_symbols[entry.data];
>> -
>> - symbol.SetByteSize(end_section_file_addr -
>> symbol_file_addr);
>> - symbol.SetSizeIsSynthesized(true);
>> + if (!symbol.GetByteSizeIsValid())
>> + {
>> + symbol.SetByteSize(end_section_file_addr -
>> symbol_file_addr);
>> + symbol.SetSizeIsSynthesized(true);
>> + }
>> }
>> }
>> }
>> @@ -1039,18 +1041,15 @@ Symtab::FindSymbolContainingFileAddress
>> return info.match_symbol;
>> }
>>
>> - const size_t symbol_byte_size = info.match_symbol->GetByteSize();
>> -
>> - if (symbol_byte_size == 0)
>> + if (!info.match_symbol->GetByteSizeIsValid())
>> {
>> - // We weren't able to find the size of the symbol so lets
>> just go
>> - // with that match we found in our search...
>> + // The matched symbol dosn't have a valid byte size so lets
>> just go with that match...
>> return info.match_symbol;
>> }
>>
>> // We were able to figure out a symbol size so lets make sure our
>> // offset puts "file_addr" in the symbol's address range.
>> - if (info.match_offset < symbol_byte_size)
>> + if (info.match_offset < info.match_symbol->GetByteSize())
>> return info.match_symbol;
>> }
>> return nullptr;
>> @@ -1066,7 +1065,11 @@ Symtab::FindSymbolContainingFileAddress
>>
>> const FileRangeToIndexMap::Entry *entry =
>> m_file_addr_to_index.FindEntryThatContains(file_addr);
>> if (entry)
>> - return SymbolAtIndex(entry->data);
>> + {
>> + Symbol* symbol = SymbolAtIndex(entry->data);
>> + if (symbol->ContainsFileAddress(file_addr))
>> + return symbol;
>> + }
>> return nullptr;
>> }
>>
>>
>>
>> _______________________________________________
>> lldb-commits mailing list
>> lldb-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>>
>
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20160120/1bf9bf9f/attachment-0001.html>
More information about the lldb-commits
mailing list