[Lldb-commits] [lldb] r258113 - Unconditionally accept symbol sizes from elf
Tamas Berghammer via lldb-commits
lldb-commits at lists.llvm.org
Thu Jan 21 03:57:38 PST 2016
Sorry for breaking the windows tests. I don't have a windows box to
investigate it but based on your description the regression is caused by my
CL while I believe the bug is in ObjectFilePECOFF (is it the one used on
Windows?).
The problem is that you have symbols with m_byte_size == 0 while calling
Symtab::FindSymbolContainingFileAddress what is invalid in almost every
case. Most likely it is caused by an issue in ObjectFilePECOFF::GetSymtab
where you don't set the size of the symbols. You either have to specify the
size of each symbol with calling Symbol::SetByteSize if this information is
stored in the the object file (done in case of elf) or
call Symtab::CalculateSymbolSizes (done in case of mach-o) after you parsed
all symbols (what will set the size of each symbol with 0 size to last
until the next symbol). Can you try out is doing one of these fixes your
issue?
Thanks,
Tamas
On Wed, Jan 20, 2016 at 11:19 PM Adrian McCarthy <amccarth at google.com>
wrote:
> 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/20160121/9c1815c0/attachment-0001.html>
More information about the lldb-commits
mailing list