<div dir="ltr">That makes sense. I'll look into ObjectFile::PECOFF.<div><br></div><div>Thanks,</div><div>Adrian.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 21, 2016 at 3:57 AM, Tamas Berghammer <span dir="ltr"><<a href="mailto:tberghammer@google.com" target="_blank">tberghammer@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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?).<div><br></div><div>The problem is that you have symbols with <span style="line-height:1.5">m_byte_size == 0 while calling </span><span style="line-height:1.5">Symtab::</span><span style="line-height:1.5">FindSymbolContainingFileAddres</span><span style="line-height:1.5">s what is invalid in almost every case. Most likely it is caused by an issue in </span>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?</div><div><br></div><div>Thanks,</div><div>Tamas</div></div><div class="HOEnZb"><div class="h5"><br><div class="gmail_quote"><div dir="ltr">On Wed, Jan 20, 2016 at 11:19 PM Adrian McCarthy <<a href="mailto:amccarth@google.com" target="_blank">amccarth@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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.<div><br></div><div>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.<br><div><br></div><div><br></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 20, 2016 at 2:33 PM, Zachary Turner via lldb-commits <span dir="ltr"><<a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Tamas,<div><br></div><div>I think this build breaks Windows. This is actually the breakage that we were discussing in the thread about thread step-over.</div><div><br></div><div>1 revision before your patch:</div><div><div>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</div><div>LLDB library dir: D:/src/llvmbuild/ninja/bin</div><div>LLDB import library dir: D:/src/llvmbuild/ninja/bin\..\lib</div><div>lldb version 3.9.0 clang revision 257802 llvm revision 257804</div><div>The 'lldb-mi' executable cannot be located. The lldb-mi tests can not be run as a result.</div><div><br></div><div>Session logs for test failures/errors/unexpected successes will go into directory 'D:/src/llvmbuild/ninja/lldb-test-traces'</div><div>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</div><div>PASS: LLDB (d:\src\llvmbuild\ninja_release\bin\clang.exe-i686) :: test_dwarf (TestSignedTypes.UnsignedTypesTestCase)</div><div>----------------------------------------------------------------------</div><div>Ran 1 test in 1.820s</div><div><br></div><div>RESULT: PASSED (1 passes, 0 failures, 0 errors, 0 skipped, 0 expected failures, 0 unexpected successes)</div></div><div><br></div><div>With your patch:</div><div><div>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</div><div>LLDB library dir: D:/src/llvmbuild/ninja/bin</div><div>LLDB import library dir: D:/src/llvmbuild/ninja/bin\..\lib</div><div>lldb version 3.9.0 clang revision 257802 llvm revision 257804</div><div>The 'lldb-mi' executable cannot be located. The lldb-mi tests can not be run as a result.</div><div><br></div><div>Session logs for test failures/errors/unexpected successes will go into directory 'D:/src/llvmbuild/ninja/lldb-test-traces'</div><div>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</div><div>FAIL: LLDB (d:\src\llvmbuild\ninja_release\bin\clang.exe-i686) :: test_dwarf (TestSignedTypes.UnsignedTypesTestCase)</div><div>======================================================================</div><div>FAIL: test_dwarf (TestSignedTypes.UnsignedTypesTestCase)</div><div> Test that variables with signed types display correctly.</div><div>----------------------------------------------------------------------</div><div>Traceback (most recent call last):</div><div> File "D:\src\llvm\tools\lldb\packages\Python\lldbsuite\test\lldbtest.py", line 2249, in dwarf_test_method</div><div> return attrvalue(self)</div><div> File "D:\src\llvm\tools\lldb\packages\Python\lldbsuite\test\lang\cpp\signed_types\TestSignedTypes.py", line 60, in test</div><div> "(long long) the_signed_long_long = 99"])</div><div> File "D:\src\llvm\tools\lldb\packages\Python\lldbsuite\test\lldbtest.py", line 2701, in expect</div><div> msg if msg else EXP_MSG(str, exe))</div><div>AssertionError: False is not True : Variable(s) displayed correctly</div><div>Config=i686-d:\src\llvmbuild\ninja_release\bin\clang.exe</div><div>----------------------------------------------------------------------</div><div>Ran 1 test in 1.641s</div><div><br></div><div>RESULT: FAILED (0 passes, 1 failures, 0 errors, 0 skipped, 0 expected failures, 0 unexpected successes)</div></div><div><br></div><div><br></div><div>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?</div></div><div><div><br><div class="gmail_quote"><div dir="ltr">On Tue, Jan 19, 2016 at 2:28 AM Tamas Berghammer via lldb-commits <<a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: tberghammer<br>
Date: Tue Jan 19 04:24:51 2016<br>
New Revision: 258113<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=258113&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=258113&view=rev</a><br>
Log:<br>
Unconditionally accept symbol sizes from elf<br>
<br>
The ELF symbol table always contain the size of the symbols so we<br>
don't have to try to guess them based on the address of the next<br>
symbol (it is needed for mach-o).<br>
<br>
The change fixes an issue when a symbol is removed after a 0 size<br>
symbol (e.g. because the second one is not public) what previously<br>
caused the symbol lookup algorithm to end up with showing the 0 size<br>
symbol even for the later addresses (what are not part of any symbol).<br>
That symbol lookup error can confuse the user and also confuses the<br>
current stack unwinder.<br>
<br>
Re-commit this CL after fixing the issue with gcc-4.9.2 on i386 Linux.<br>
<br>
Differential revision: <a href="http://reviews.llvm.org/D16186" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16186</a><br>
<br>
Modified:<br>
lldb/trunk/include/lldb/Symbol/Symbol.h<br>
lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp<br>
lldb/trunk/source/Symbol/Symbol.cpp<br>
lldb/trunk/source/Symbol/Symtab.cpp<br>
<br>
Modified: lldb/trunk/include/lldb/Symbol/Symbol.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Symbol/Symbol.h?rev=258113&r1=258112&r2=258113&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Symbol/Symbol.h?rev=258113&r1=258112&r2=258113&view=diff</a><br>
==============================================================================<br>
--- lldb/trunk/include/lldb/Symbol/Symbol.h (original)<br>
+++ lldb/trunk/include/lldb/Symbol/Symbol.h Tue Jan 19 04:24:51 2016<br>
@@ -383,6 +383,9 @@ public:<br>
bool prefer_file_cache,<br>
Stream &strm);<br>
<br>
+ bool<br>
+ ContainsFileAddress (lldb::addr_t file_addr) const;<br>
+<br>
protected:<br>
// This is the internal guts of ResolveReExportedSymbol, it assumes reexport_name is not null, and that module_spec<br>
// is valid. We track the modules we've already seen to make sure we don't get caught in a cycle.<br>
<br>
Modified: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp?rev=258113&r1=258112&r2=258113&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp?rev=258113&r1=258112&r2=258113&view=diff</a><br>
==============================================================================<br>
--- lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp (original)<br>
+++ lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp Tue Jan 19 04:24:51 2016<br>
@@ -2283,6 +2283,12 @@ ObjectFileELF::ParseSymbols (Symtab *sym<br>
mangled.SetDemangledName( ConstString((demangled_name + suffix).str()) );<br>
}<br>
<br>
+ // In ELF all symbol should have a valid size but it is not true for some code symbols<br>
+ // coming from hand written assembly. As none of the code symbol should have 0 size we try<br>
+ // to calculate the size for these symbols in the symtab with saying that their original<br>
+ // size is not valid.<br>
+ bool symbol_size_valid = symbol.st_size != 0 || symbol_type != eSymbolTypeCode;<br>
+<br>
Symbol dc_symbol(<br>
i + start_id, // ID is the original symbol table index.<br>
mangled,<br>
@@ -2295,7 +2301,7 @@ ObjectFileELF::ParseSymbols (Symtab *sym<br>
symbol_section_sp, // Section in which this symbol is defined or null.<br>
symbol_value, // Offset in section or symbol value.<br>
symbol.st_size), // Size in bytes of this symbol.<br>
- symbol.st_size != 0, // Size is valid if it is not 0<br>
+ symbol_size_valid, // Symbol size is valid<br>
has_suffix, // Contains linker annotations?<br>
flags); // Symbol flags.<br>
symtab->AddSymbol(dc_symbol);<br>
@@ -2304,7 +2310,9 @@ ObjectFileELF::ParseSymbols (Symtab *sym<br>
}<br>
<br>
unsigned<br>
-ObjectFileELF::ParseSymbolTable(Symtab *symbol_table, user_id_t start_id, lldb_private::Section *symtab)<br>
+ObjectFileELF::ParseSymbolTable(Symtab *symbol_table,<br>
+ user_id_t start_id,<br>
+ lldb_private::Section *symtab)<br>
{<br>
if (symtab->GetObjectFile() != this)<br>
{<br>
<br>
Modified: lldb/trunk/source/Symbol/Symbol.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Symbol/Symbol.cpp?rev=258113&r1=258112&r2=258113&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Symbol/Symbol.cpp?rev=258113&r1=258112&r2=258113&view=diff</a><br>
==============================================================================<br>
--- lldb/trunk/source/Symbol/Symbol.cpp (original)<br>
+++ lldb/trunk/source/Symbol/Symbol.cpp Tue Jan 19 04:24:51 2016<br>
@@ -737,3 +737,10 @@ Symbol::GetDisassembly (const ExecutionC<br>
}<br>
return false;<br>
}<br>
+<br>
+bool<br>
+Symbol::ContainsFileAddress (lldb::addr_t file_addr) const<br>
+{<br>
+ return m_addr_range.ContainsFileAddress(file_addr);<br>
+}<br>
+<br>
<br>
Modified: lldb/trunk/source/Symbol/Symtab.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Symbol/Symtab.cpp?rev=258113&r1=258112&r2=258113&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Symbol/Symtab.cpp?rev=258113&r1=258112&r2=258113&view=diff</a><br>
==============================================================================<br>
--- lldb/trunk/source/Symbol/Symtab.cpp (original)<br>
+++ lldb/trunk/source/Symbol/Symtab.cpp Tue Jan 19 04:24:51 2016<br>
@@ -971,9 +971,11 @@ Symtab::InitAddressIndexes()<br>
if (end_section_file_addr > symbol_file_addr)<br>
{<br>
Symbol &symbol = m_symbols[entry.data];<br>
-<br>
- symbol.SetByteSize(end_section_file_addr - symbol_file_addr);<br>
- symbol.SetSizeIsSynthesized(true);<br>
+ if (!symbol.GetByteSizeIsValid())<br>
+ {<br>
+ symbol.SetByteSize(end_section_file_addr - symbol_file_addr);<br>
+ symbol.SetSizeIsSynthesized(true);<br>
+ }<br>
}<br>
}<br>
}<br>
@@ -1039,18 +1041,15 @@ Symtab::FindSymbolContainingFileAddress<br>
return info.match_symbol;<br>
}<br>
<br>
- const size_t symbol_byte_size = info.match_symbol->GetByteSize();<br>
-<br>
- if (symbol_byte_size == 0)<br>
+ if (!info.match_symbol->GetByteSizeIsValid())<br>
{<br>
- // We weren't able to find the size of the symbol so lets just go<br>
- // with that match we found in our search...<br>
+ // The matched symbol dosn't have a valid byte size so lets just go with that match...<br>
return info.match_symbol;<br>
}<br>
<br>
// We were able to figure out a symbol size so lets make sure our<br>
// offset puts "file_addr" in the symbol's address range.<br>
- if (info.match_offset < symbol_byte_size)<br>
+ if (info.match_offset < info.match_symbol->GetByteSize())<br>
return info.match_symbol;<br>
}<br>
return nullptr;<br>
@@ -1066,7 +1065,11 @@ Symtab::FindSymbolContainingFileAddress<br>
<br>
const FileRangeToIndexMap::Entry *entry = m_file_addr_to_index.FindEntryThatContains(file_addr);<br>
if (entry)<br>
- return SymbolAtIndex(entry->data);<br>
+ {<br>
+ Symbol* symbol = SymbolAtIndex(entry->data);<br>
+ if (symbol->ContainsFileAddress(file_addr))<br>
+ return symbol;<br>
+ }<br>
return nullptr;<br>
}<br>
<br>
<br>
<br>
_______________________________________________<br>
lldb-commits mailing list<br>
<a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits</a><br>
</blockquote></div>
</div></div><br>_______________________________________________<br>
lldb-commits mailing list<br>
<a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits</a><br>
<br></blockquote></div><br></div>
</blockquote></div>
</div></div></blockquote></div><br></div>