[Lldb-commits] [lldb] r258113 - Unconditionally accept symbol sizes from elf
Adrian McCarthy via lldb-commits
lldb-commits at lists.llvm.org
Thu Jan 21 07:43:15 PST 2016
That makes sense. I'll look into ObjectFile::PECOFF.
Thanks,
Adrian.
On Thu, Jan 21, 2016 at 3:57 AM, Tamas Berghammer <tberghammer at google.com>
wrote:
> 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/e6714a63/attachment-0001.html>
More information about the lldb-commits
mailing list