[Lldb-commits] [PATCH] D40587: [lldb] Minor fixes in TaskPool
labath added a comment. Looks good, thanks. Repository: rL LLVM https://reviews.llvm.org/D40587 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D40539: Allow using the object file name for debug symbol resolution
On 28 November 2017 at 17:28, Zachary Turner via lldb-commits wrote: > > > On Tue, Nov 28, 2017 at 2:48 AM Pavel Labath via Phabricator via > lldb-commits wrote: >> >> labath added a comment. >> >> On 28 November 2017 at 06:12, Zachary Turner via lldb-commits >> wrote: >> >> > yaml2core would be an excellent idea for a tool. >> >> An (elf) core file is an elf file, with metadata in the elf program >> headers, so I think this would naturally fit into yaml2obj. Unfortunately, >> yaml2obj currently does not support program headers (but there is a TODO to >> add that). If that TODO is fixed, we would have a yaml representation of >> both core files and elf executables (which we also can't do because they >> have program headers, and yaml2obj strips them). > > > yes but a windows core file (i.e. minidump) is not a coff file. So I think > a tool like yaml2core would be useful, and for the ELF case it can simply > link against the same code that yaml2obj is using for elv. > Fair enough. I don't really care about the name, I just wanted to make sure someone doesn't go off reimplementing elf serialization, when there isn't much missing from yaml2obj to make that work. ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40587: [lldb] Minor fixes in TaskPool
fjricci added inline comments. Comment at: source/Host/common/TaskPool.cpp:55 + static const unsigned g_hardware_concurrency = +std::max(1u, std::thread::hardware_concurrency()); + return g_hardware_concurrency; Is 1 the best default here when hardware_concurrency() isn't computable? Seems like it could have some big performance implications, and I'm not sure how common it is for people to debug on host machines that only support one thread. Repository: rL LLVM https://reviews.llvm.org/D40587 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40615: Fix assertion in ClangASTContext
labath created this revision. llvm::APSInt(0) asserts because it creates an int with bit-width 0 and not (as I thought) a value 0. Theoretically it should be sufficient to change this to APSInt(1), as the intention there was that the value of the first argument should be ignored if the type is invalid, but that would look dodgy. Instead, I use llvm::Optional to denote an invalid value and use a special struct instead of a std::pair, to reduce typing and increase clarity. https://reviews.llvm.org/D40615 Files: include/lldb/Symbol/ClangASTContext.h include/lldb/Symbol/CompilerType.h include/lldb/Symbol/TypeSystem.h source/API/SBType.cpp source/Plugins/Language/CPlusPlus/LibCxxBitset.cpp source/Symbol/ClangASTContext.cpp source/Symbol/CompilerType.cpp source/Symbol/TypeSystem.cpp unittests/Symbol/TestClangASTContext.cpp Index: unittests/Symbol/TestClangASTContext.cpp === --- unittests/Symbol/TestClangASTContext.cpp +++ unittests/Symbol/TestClangASTContext.cpp @@ -382,8 +382,8 @@ infos.names.push_back("T"); infos.args.push_back(TemplateArgument(m_ast->getASTContext()->IntTy)); infos.names.push_back("I"); - infos.args.push_back(TemplateArgument(*m_ast->getASTContext(), -llvm::APSInt(47), + llvm::APSInt arg(llvm::APInt(8, 47)); + infos.args.push_back(TemplateArgument(*m_ast->getASTContext(), arg, m_ast->getASTContext()->IntTy)); // template struct foo; @@ -419,15 +419,16 @@ eTemplateArgumentKindType); EXPECT_EQ(m_ast->GetTypeTemplateArgument(t.GetOpaqueQualType(), 0), int_type); -auto p = m_ast->GetIntegralTemplateArgument(t.GetOpaqueQualType(), 0); -EXPECT_EQ(p.second, CompilerType()); +auto result = m_ast->GetIntegralTemplateArgument(t.GetOpaqueQualType(), 0); +EXPECT_EQ(llvm::None, result); EXPECT_EQ(m_ast->GetTemplateArgumentKind(t.GetOpaqueQualType(), 1), eTemplateArgumentKindIntegral); EXPECT_EQ(m_ast->GetTypeTemplateArgument(t.GetOpaqueQualType(), 1), CompilerType()); -p = m_ast->GetIntegralTemplateArgument(t.GetOpaqueQualType(), 1); -EXPECT_EQ(p.first, llvm::APSInt(47)); -EXPECT_EQ(p.second, int_type); +result = m_ast->GetIntegralTemplateArgument(t.GetOpaqueQualType(), 1); +ASSERT_NE(llvm::None, result); +EXPECT_EQ(arg, result->value); +EXPECT_EQ(int_type, result->type); } } Index: source/Symbol/TypeSystem.cpp === --- source/Symbol/TypeSystem.cpp +++ source/Symbol/TypeSystem.cpp @@ -111,10 +111,10 @@ return CompilerType(); } -std::pair +llvm::Optional TypeSystem::GetIntegralTemplateArgument(opaque_compiler_type_t type, -size_t idx) { - return {llvm::APSInt(0), CompilerType()}; +size_t idx) { + return llvm::None; } LazyBool TypeSystem::ShouldPrintAsOneLiner(void *type, ValueObject *valobj) { Index: source/Symbol/CompilerType.cpp === --- source/Symbol/CompilerType.cpp +++ source/Symbol/CompilerType.cpp @@ -703,12 +703,11 @@ return CompilerType(); } -std::pair -CompilerType::GetIntegralTemplateArgument(size_t idx) const -{ +llvm::Optional +CompilerType::GetIntegralTemplateArgument(size_t idx) const { if (IsValid()) return m_type_system->GetIntegralTemplateArgument(m_type, idx); - return {llvm::APSInt(0), CompilerType()}; + return llvm::None; } CompilerType CompilerType::GetTypeForFormatters() const { Index: source/Symbol/ClangASTContext.cpp === --- source/Symbol/ClangASTContext.cpp +++ source/Symbol/ClangASTContext.cpp @@ -7650,21 +7650,21 @@ return CompilerType(getASTContext(), template_arg.getAsType()); } -std::pair +llvm::Optional ClangASTContext::GetIntegralTemplateArgument(lldb::opaque_compiler_type_t type, size_t idx) { const clang::ClassTemplateSpecializationDecl *template_decl = GetAsTemplateSpecialization(type); if (! template_decl || idx >= template_decl->getTemplateArgs().size()) -return {llvm::APSInt(0), CompilerType()}; +return llvm::None; const clang::TemplateArgument &template_arg = template_decl->getTemplateArgs()[idx]; if (template_arg.getKind() != clang::TemplateArgument::Integral) -return {llvm::APSInt(0), CompilerType()}; +return llvm::None; - return {template_arg.getAsIntegral(), - CompilerType(getASTContext(), template_arg.getIntegralType())}; + return {{template_arg.getAsIntegral(), + CompilerType(getASTContext(), template_arg.getIntegralType())}}; } CompilerType ClangASTContext::GetTypeForFormatters(void *type) { Index: source/Plugins/Language/CPlusPlus/LibCxxBitset.cp
[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
labath created this revision. Herald added subscribers: aprantl, mgorny, emaste. We use the llvm decompressor to decompress SHF_COMPRESSED sections. This enables us to read data from debug info sections, which are sometimes compressed, particuarly in the split-dwarf case. This functionality is only available if llvm is compiled with zlib support. https://reviews.llvm.org/D40616 Files: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp source/Plugins/ObjectFile/ELF/ObjectFileELF.h unittests/ObjectFile/ELF/CMakeLists.txt unittests/ObjectFile/ELF/Inputs/compressed-sections.yaml unittests/ObjectFile/ELF/TestObjectFileELF.cpp Index: unittests/ObjectFile/ELF/TestObjectFileELF.cpp === --- unittests/ObjectFile/ELF/TestObjectFileELF.cpp +++ unittests/ObjectFile/ELF/TestObjectFileELF.cpp @@ -10,12 +10,13 @@ #include "Plugins/ObjectFile/ELF/ObjectFileELF.h" #include "Plugins/SymbolVendor/ELF/SymbolVendorELF.h" +#include "TestingSupport/TestUtilities.h" #include "lldb/Core/Module.h" #include "lldb/Core/ModuleSpec.h" #include "lldb/Core/Section.h" #include "lldb/Host/HostInfo.h" -#include "TestingSupport/TestUtilities.h" #include "llvm/ADT/Optional.h" +#include "llvm/Support/Compression.h" #include "llvm/Support/FileUtilities.h" #include "llvm/Support/Path.h" #include "llvm/Support/Program.h" @@ -97,3 +98,42 @@ ASSERT_NE(nullptr, start); EXPECT_EQ(text_sp, start->GetAddress().GetSection()); } + +TEST_F(ObjectFileELFTest, CompressedSections) { + if (!llvm::zlib::isAvailable()) { +GTEST_LOG_(WARNING) +<< "Test skipped because the decompression library is not available."; +return; + } + std::string yaml = GetInputFilePath("compressed-sections.yaml"); + llvm::SmallString<128> obj; + ASSERT_NO_ERROR(llvm::sys::fs::createTemporaryFile( + "compressed-sections-%%", "obj", obj)); + + llvm::FileRemover remover(obj); + const char *args[] = {YAML2OBJ, yaml.c_str(), nullptr}; + llvm::StringRef obj_ref = obj; + const llvm::Optional redirects[] = {llvm::None, obj_ref, + llvm::None}; + ASSERT_EQ(0, llvm::sys::ExecuteAndWait(YAML2OBJ, args, nullptr, redirects)); + uint64_t size; + ASSERT_NO_ERROR(llvm::sys::fs::file_size(obj, size)); + ASSERT_GT(size, 0u); + + ModuleSpec spec{FileSpec(obj, false)}; + spec.GetSymbolFileSpec().SetFile(obj, false); + auto module_sp = std::make_shared(spec); + SectionList *list = module_sp->GetSectionList(); + ASSERT_NE(nullptr, list); + + auto hello_elf_sp = list->FindSectionByName(ConstString(".hello_elf")); + ASSERT_NE(nullptr, hello_elf_sp); + DataExtractor data; + ASSERT_EQ(9u, hello_elf_sp->GetSectionData(data)); + offset_t offset = 0; + EXPECT_STREQ("Hello ELF", data.GetCStr(&offset)); + + auto bogus_sp = list->FindSectionByName(ConstString(".bogus")); + ASSERT_NE(nullptr, bogus_sp); + EXPECT_EQ(0u, bogus_sp->GetByteSize()); +} Index: unittests/ObjectFile/ELF/Inputs/compressed-sections.yaml === --- /dev/null +++ unittests/ObjectFile/ELF/Inputs/compressed-sections.yaml @@ -0,0 +1,15 @@ +--- !ELF +FileHeader: + Class: ELFCLASS32 + Data:ELFDATA2LSB + Type:ET_REL + Machine: EM_386 +Sections: + - Name:.hello_elf +Type:SHT_PROGBITS +Flags: [ SHF_COMPRESSED ] +Content: 010009000100789cf348cdc9c95770f57103000f8d02ec + - Name:.bogus +Type:SHT_PROGBITS +Flags: [ SHF_COMPRESSED ] +Content: deadbeefbaadf00d Index: unittests/ObjectFile/ELF/CMakeLists.txt === --- unittests/ObjectFile/ELF/CMakeLists.txt +++ unittests/ObjectFile/ELF/CMakeLists.txt @@ -14,5 +14,6 @@ set(test_inputs sections-resolve-consistently.yaml + compressed-sections.yaml ) add_unittest_inputs(ObjectFileELFTests "${test_inputs}") Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.h === --- source/Plugins/ObjectFile/ELF/ObjectFileELF.h +++ source/Plugins/ObjectFile/ELF/ObjectFileELF.h @@ -140,6 +140,9 @@ ObjectFile::Strata CalculateStrata() override; + size_t ReadSectionData(lldb_private::Section *section, + lldb_private::DataExtractor §ion_data) override; + // Returns number of program headers found in the ELF file. size_t GetProgramHeaderCount(); Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp === --- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp +++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp @@ -23,14 +23,16 @@ #include "lldb/Target/SectionLoadList.h" #include "lldb/Target/Target.h" #include "lldb/Utility/ArchSpec.h" +#include "lldb/Utility/DataBufferHeap.h" #in
[Lldb-commits] [PATCH] D40615: Fix assertion in ClangASTContext
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Few nits, but nothing that would hold up the patch. Looks good. Comment at: include/lldb/Symbol/CompilerType.h:294 + struct IntegralTemplateArgument; + Can't you just define the type right here? Comment at: source/Plugins/Language/CPlusPlus/LibCxxBitset.cpp:62-63 size_t size = 0; - auto value_and_type = - m_backend.GetCompilerType().GetIntegralTemplateArgument(0); - if (value_and_type.second) -size = value_and_type.first.getLimitedValue(capping_size); + auto arg = m_backend.GetCompilerType().GetIntegralTemplateArgument(0); + if (arg) +size = arg->value.getLimitedValue(capping_size); Should we combine these two lines? ``` if (auto arg = m_backend.GetCompilerType().GetIntegralTemplateArgument(0)) ``` https://reviews.llvm.org/D40615 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
zturner added a comment. It's too bad this has to be written as a unit test, because this would be the perfect candidate for a FileCheck style test. Probably a long shot, but have you tried the llvm-lit project? Last time I tried it, it basically worked, but there were only a handful of tests in it. It might be possible to write a test in such a way that it invokes `lldb` with a `.lldbinit` file which enables logging to a file, ends with the `quit` command, and then FileChecks the log file. https://reviews.llvm.org/D40616 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
clayborg added a comment. Why would we text scrape when we can test the API? https://reviews.llvm.org/D40616 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
zturner added a comment. For the same reason that the entire rest of the LLVM project and all other subprojects do it, when it makes sense and the nature of the test lends itself to it. https://reviews.llvm.org/D40616 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
zturner added a comment. Note that there's no interactivity here. This is "feed some input, get some output, make sure the output is correct". That's exactly what FileCheck is designed for. This isn't even testing the public API, it's testing the private API. We should prefer testing the actual program in this case. https://reviews.llvm.org/D40616 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
davide requested changes to this revision. davide added a comment. This revision now requires changes to proceed. This one is a little weird when written as unittest. Not the worst thing, but I agree it should use `llvm-lit`. Can you give it a shot, Pavel? If that doesn't work, we should at least evaluate the amount of work needed to get `llvm-lit` to run with `lldb` before dismissing it entirely. BTW, nice to see lldb getting more and more tests, regardless :) https://reviews.llvm.org/D40616 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
davide added a comment. Another very good reason for writing a `FileCheck` test rather than a unittest is that writing unittest is tedious :) In particular for new contributors, FileCheck tests are much easier to write and in this case testing the API surface doesn't seem to add much value. https://reviews.llvm.org/D40616 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
jingham added a comment. It does look a little weird as a unit test, but to me this is mostly because it would been much simpler to write it as a regular SB API test. Anyway, I really don't want the details of the text output of lldb commands to become API. Our experience with gdb was that over time as you write more and more tests that scrape text output, you end up not being able to change command output because the burden of fixing up all the tests becomes too onerous. You can use text scraping in the current lldb testsuite. We discourage that for the reasons above, and try to isolate the tests that do so by having lldbutils interfaces to do the explicit scraping. But it is just as easy, and quite often much easier, to examine objects directly in the lldb testsuite, so this mechanism encourages virtue, even though it doesn't enforce it. OTOH adding a test mechanism that explicitly relies only on command output scraping leads us down the path that ended up being a real PITN for the gdb testsuite. So for that reason I am not in favor of going this way. https://reviews.llvm.org/D40616 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
We’ve had this discussion several times, but at the end of the day nothing has really changed with the larger llvm project having adopted this model and having spent significant effort in moving forward with it. Normally, the reason we don't use this model for LLDB tests is because they require interactivity, and if need be I can find several threads where the response has been, specifically "FileCheck tests aren't a good fit for LLDB because of the interactivity and the nature of the test doesn't lend itself well to textual output comparison". So here we have a case where there is no interactivity, and in fact the test lends itself perfect to textual comparison. With all due respect, given that the previously stated reasons for preferring not to use text-scraping tests don't apply in this case, combined with the fact that there is a strong desire by the larger project to use a standard, cross-project testing infrastructure that is understood by several hundred developers instead of several, I don't see a good reason to not at least attempt this route in cases where it makes sense. On Wed, Nov 29, 2017 at 12:07 PM Jim Ingham via Phabricator < revi...@reviews.llvm.org> wrote: > jingham added a comment. > > It does look a little weird as a unit test, but to me this is mostly > because it would been much simpler to write it as a regular SB API test. > > Anyway, I really don't want the details of the text output of lldb > commands to become API. Our experience with gdb was that over time as you > write more and more tests that scrape text output, you end up not being > able to change command output because the burden of fixing up all the tests > becomes too onerous. You can use text scraping in the current lldb > testsuite. We discourage that for the reasons above, and try to isolate > the tests that do so by having lldbutils interfaces to do the explicit > scraping. But it is just as easy, and quite often much easier, to examine > objects directly in the lldb testsuite, so this mechanism encourages > virtue, even though it doesn't enforce it. > > OTOH adding a test mechanism that explicitly relies only on command output > scraping leads us down the path that ended up being a real PITN for the gdb > testsuite. So for that reason I am not in favor of going this way. > > > https://reviews.llvm.org/D40616 > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40466: DWZ 03/12: DWARFCompileUnit split to DWARFCompileUnitData
jankratochvil marked an inline comment as done. jankratochvil added inline comments. Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:216 + DWARFCompileUnitData *m_data; + clayborg wrote: > Is there a reason this is a member variable that I am not seeing? Seems we > could have this class inherit from DWARFCompileUnitData. I am guessing this > will be needed for a future patch? Yes, future patch D40474 contains a new constructor so multiple `DWARFCompileUnit` then point to single `DWARFCompileUnitData`. Sure that happens only in the case ot `DW_TAG_partial_unit` (one `DWARFCompileUnit` is read from a file while other `DWARFCompileUnit` are remapped instances with unique offset as used from units which did use `DW_TAG_imported_unit` for them). `DWARFCompileUnit(DWARFCompileUnitData *data, DWARFCompileUnit *main_cu);` https://reviews.llvm.org/D40466 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40467: DWZ 04/12: Separate Offset also into FileOffset
jankratochvil marked an inline comment as done. jankratochvil added inline comments. Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h:15 #include +#include clayborg wrote: > Why is this added here? Doesn't seem to be needed, or it should be moved to > .cpp file? Yes, a patch split up typo, sorry. https://reviews.llvm.org/D40467 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40467: DWZ 04/12: Separate Offset also into FileOffset
jankratochvil updated this revision to Diff 124803. jankratochvil marked an inline comment as done. jankratochvil added a comment. Removed excessive: #include: https://reviews.llvm.org/D40467 Files: include/lldb/Expression/DWARFExpression.h source/Expression/DWARFExpression.cpp source/Plugins/SymbolFile/DWARF/DWARFAttribute.cpp source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp source/Plugins/SymbolFile/DWARF/DWARFFormValue.h source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp === --- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp +++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp @@ -3812,7 +3812,8 @@ DWARFFormValue::GetFixedFormSizesForAddressSize( attributes.CompileUnitAtIndex(i)->GetAddressByteSize(), attributes.CompileUnitAtIndex(i)->IsDWARF64()); -uint32_t data_offset = attributes.DIEOffsetAtIndex(i); +uint32_t data_file_offset = die.GetCU() +->ThisCUUniqToFileOffset(attributes.DIEOffsetAtIndex(i)); uint32_t data_length = fixed_form_sizes.GetSize(form_value.Form()); if (data_length == 0) { @@ -3825,21 +3826,22 @@ const_value = form_value; } } else - location.CopyOpcodeData(module, debug_info_data, data_offset, - data_length); + location.CopyOpcodeData(module, debug_info_data, + data_file_offset, data_length); } else { // Retrieve the value as a string expression. if (form_value.Form() == DW_FORM_strp) { DWARFFormValue::FixedFormSizes fixed_form_sizes = DWARFFormValue::GetFixedFormSizesForAddressSize( attributes.CompileUnitAtIndex(i) ->GetAddressByteSize(), attributes.CompileUnitAtIndex(i)->IsDWARF64()); - uint32_t data_offset = attributes.DIEOffsetAtIndex(i); + uint32_t data_file_offset = die.GetCU() + ->ThisCUUniqToFileOffset(attributes.DIEOffsetAtIndex(i)); uint32_t data_length = fixed_form_sizes.GetSize(form_value.Form()); - location.CopyOpcodeData(module, debug_info_data, data_offset, - data_length); + location.CopyOpcodeData(module, debug_info_data, + data_file_offset, data_length); } else { const char *str = form_value.AsCString(); uint32_t string_offset = Index: source/Plugins/SymbolFile/DWARF/DWARFFormValue.h === --- source/Plugins/SymbolFile/DWARF/DWARFFormValue.h +++ source/Plugins/SymbolFile/DWARF/DWARFFormValue.h @@ -63,8 +63,9 @@ const ValueType &Value() const { return m_value; } void Dump(lldb_private::Stream &s) const; bool ExtractValue(const lldb_private::DWARFDataExtractor &data, -lldb::offset_t *offset_ptr); +lldb::offset_t *file_offset_ptr); const uint8_t *BlockData() const; + uint64_t ReferenceInFile() const; uint64_t Reference() const; uint64_t Reference(dw_offset_t offset) const; bool Boolean() const { return m_value.value.uval != 0; } @@ -76,10 +77,11 @@ dw_addr_t Address() const; bool IsValid() const { return m_form != 0; } bool SkipValue(const lldb_private::DWARFDataExtractor &debug_info_data, - lldb::offset_t *offset_ptr) const; + lldb::offset_t *file_offset_ptr) const; static bool SkipValue(const dw_form_t form, const lldb_private::DWARFDataExtractor &debug_info_data, -lldb::offset_t *offset_ptr, const DWARFCompileUnit *cu); +lldb::offset_t *file_offset_ptr, +const DWARFCompileUnit *cu); static bool IsBlockForm(const dw_form_t form); static bool IsDataForm(const dw_form_t form); static FixedFormSizes GetFixedFormSizesForAddressSize(uint8_t addr_size, Index: source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp === --- source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp +++ source/Plugins/Sy
[Lldb-commits] [PATCH] D40212: DWZ 02/12: refactor: Unify+simplify DWARFCompileUnit ctor+Clear() into in-class initializers + Extract()
jankratochvil added inline comments. Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp:248-249 -while (cu->Extract(dwarf2Data->get_debug_info_data(), &offset)) { +for (;;) { + DWARFCompileUnitSP cu = DWARFCompileUnit::Extract(dwarf2Data, &offset); + if (cu.get() == NULL) clayborg wrote: > Might be cleaner to do: > ``` > DWARFCompileUnitSP cu; > while ((cu = DWARFCompileUnit::Extract(dwarf2Data, &offset))) { > ``` > I did not know LLVM style can be this brief, that is sure better, implemented. https://reviews.llvm.org/D40212 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r319359 - refactor: Unify+simplify DWARFCompileUnit ctor+Clear() into in-class initializers + Extract()
Author: jankratochvil Date: Wed Nov 29 13:13:11 2017 New Revision: 319359 URL: http://llvm.org/viewvc/llvm-project?rev=319359&view=rev Log: refactor: Unify+simplify DWARFCompileUnit ctor+Clear() into in-class initializers + Extract() It has no functionality effect. I was concerned about the worse performance of DWARFDebugInfo::Parse this way of allocating+destroying a CU for each iteration but I see it is now used only by DWARFDebugInfo::Dump so that is no longer a problem. Differential revision: https://reviews.llvm.org/D40212 Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp?rev=319359&r1=319358&r2=319359&view=diff == --- lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp (original) +++ lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp Wed Nov 29 13:13:11 2017 @@ -39,68 +39,48 @@ using namespace std; extern int g_verbose; DWARFCompileUnit::DWARFCompileUnit(SymbolFileDWARF *dwarf2Data) -: m_dwarf2Data(dwarf2Data), m_abbrevs(NULL), m_user_data(NULL), - m_die_array(), m_func_aranges_ap(), m_base_addr(0), - m_offset(DW_INVALID_OFFSET), m_length(0), m_version(0), - m_addr_size(DWARFCompileUnit::GetDefaultAddressSize()), - m_producer(eProducerInvalid), m_producer_version_major(0), - m_producer_version_minor(0), m_producer_version_update(0), - m_language_type(eLanguageTypeUnknown), m_is_dwarf64(false), - m_is_optimized(eLazyBoolCalculate), m_addr_base(0), - m_ranges_base(0), m_base_obj_offset(DW_INVALID_OFFSET) {} +: m_dwarf2Data(dwarf2Data) {} DWARFCompileUnit::~DWARFCompileUnit() {} -void DWARFCompileUnit::Clear() { - m_offset = DW_INVALID_OFFSET; - m_length = 0; - m_version = 0; - m_abbrevs = NULL; - m_addr_size = DWARFCompileUnit::GetDefaultAddressSize(); - m_base_addr = 0; - m_die_array.clear(); - m_func_aranges_ap.reset(); - m_user_data = NULL; - m_producer = eProducerInvalid; - m_language_type = eLanguageTypeUnknown; - m_is_dwarf64 = false; - m_is_optimized = eLazyBoolCalculate; - m_addr_base = 0; - m_base_obj_offset = DW_INVALID_OFFSET; -} +DWARFCompileUnitSP DWARFCompileUnit::Extract(SymbolFileDWARF *dwarf2Data, +lldb::offset_t *offset_ptr) { + DWARFCompileUnitSP cu_sp(new DWARFCompileUnit(dwarf2Data)); + // Out of memory? + if (cu_sp.get() == NULL) +return nullptr; -bool DWARFCompileUnit::Extract(const DWARFDataExtractor &debug_info, - lldb::offset_t *offset_ptr) { - Clear(); + const DWARFDataExtractor &debug_info = dwarf2Data->get_debug_info_data(); - m_offset = *offset_ptr; + cu_sp->m_offset = *offset_ptr; if (debug_info.ValidOffset(*offset_ptr)) { dw_offset_t abbr_offset; -const DWARFDebugAbbrev *abbr = m_dwarf2Data->DebugAbbrev(); -m_length = debug_info.GetDWARFInitialLength(offset_ptr); -m_is_dwarf64 = debug_info.IsDWARF64(); -m_version = debug_info.GetU16(offset_ptr); +const DWARFDebugAbbrev *abbr = dwarf2Data->DebugAbbrev(); +cu_sp->m_length = debug_info.GetDWARFInitialLength(offset_ptr); +cu_sp->m_is_dwarf64 = debug_info.IsDWARF64(); +cu_sp->m_version = debug_info.GetU16(offset_ptr); abbr_offset = debug_info.GetDWARFOffset(offset_ptr); -m_addr_size = debug_info.GetU8(offset_ptr); +cu_sp->m_addr_size = debug_info.GetU8(offset_ptr); -bool length_OK = debug_info.ValidOffset(GetNextCompileUnitOffset() - 1); -bool version_OK = SymbolFileDWARF::SupportedVersion(m_version); +bool length_OK = +debug_info.ValidOffset(cu_sp->GetNextCompileUnitOffset() - 1); +bool version_OK = SymbolFileDWARF::SupportedVersion(cu_sp->m_version); bool abbr_offset_OK = -m_dwarf2Data->get_debug_abbrev_data().ValidOffset(abbr_offset); -bool addr_size_OK = ((m_addr_size == 4) || (m_addr_size == 8)); +dwarf2Data->get_debug_abbrev_data().ValidOffset(abbr_offset); +bool addr_size_OK = (cu_sp->m_addr_size == 4) || (cu_sp->m_addr_size == 8); if (length_OK && version_OK && addr_size_OK && abbr_offset_OK && abbr != NULL) { - m_abbrevs = abbr->GetAbbreviationDeclarationSet(abbr_offset); - return true; + cu_sp->m_abbrevs = abbr->GetAbbreviationDeclarationSet(abbr_offset); + return cu_sp; } // reset the offset to where we tried to parse from if anything went wrong -*offset_ptr = m_offset; +*offset_ptr = cu_sp->m_offset; } - return false; + return nullptr; } void DWARFCompileUnit::ClearDIEs(bool keep_compile_unit_die) { Modified: lldb/tru
[Lldb-commits] [PATCH] D40212: DWZ 02/12: refactor: Unify+simplify DWARFCompileUnit ctor+Clear() into in-class initializers + Extract()
This revision was automatically updated to reflect the committed changes. Closed by commit rL319359: refactor: Unify+simplify DWARFCompileUnit ctor+Clear() into in-class… (authored by jankratochvil). Changed prior to commit: https://reviews.llvm.org/D40212?vs=124290&id=124806#toc Repository: rL LLVM https://reviews.llvm.org/D40212 Files: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h Index: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h === --- lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h +++ lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h @@ -18,6 +18,8 @@ class SymbolFileDWARF; class SymbolFileDWARFDwo; +typedef std::shared_ptr DWARFCompileUnitSP; + class DWARFCompileUnit { public: enum Producer { @@ -28,17 +30,15 @@ eProcucerOther }; - DWARFCompileUnit(SymbolFileDWARF *dwarf2Data); + static DWARFCompileUnitSP Extract(SymbolFileDWARF *dwarf2Data, + lldb::offset_t *offset_ptr); ~DWARFCompileUnit(); - bool Extract(const lldb_private::DWARFDataExtractor &debug_info, - lldb::offset_t *offset_ptr); size_t ExtractDIEsIfNeeded(bool cu_die_only); DWARFDIE LookupAddress(const dw_addr_t address); size_t AppendDIEsWithTag(const dw_tag_t tag, DWARFDIECollection &matching_dies, uint32_t depth = UINT32_MAX) const; - void Clear(); bool Verify(lldb_private::Stream *s) const; void Dump(lldb_private::Stream *s) const; // Offset of the initial length field. @@ -163,33 +163,33 @@ SymbolFileDWARF *m_dwarf2Data; std::unique_ptr m_dwo_symbol_file; const DWARFAbbreviationDeclarationSet *m_abbrevs; - void *m_user_data; + void *m_user_data = nullptr; DWARFDebugInfoEntry::collection m_die_array; // The compile unit debug information entry item std::unique_ptr m_func_aranges_ap; // A table similar to // the .debug_aranges // table, but this one // points to the exact // DW_TAG_subprogram // DIEs - dw_addr_t m_base_addr; + dw_addr_t m_base_addr = 0; // Offset of the initial length field. dw_offset_t m_offset; dw_offset_t m_length; uint16_t m_version; uint8_t m_addr_size; - Producer m_producer; - uint32_t m_producer_version_major; - uint32_t m_producer_version_minor; - uint32_t m_producer_version_update; - lldb::LanguageType m_language_type; + Producer m_producer = eProducerInvalid; + uint32_t m_producer_version_major = 0; + uint32_t m_producer_version_minor = 0; + uint32_t m_producer_version_update = 0; + lldb::LanguageType m_language_type = lldb::eLanguageTypeUnknown; bool m_is_dwarf64; - lldb_private::LazyBool m_is_optimized; - dw_addr_t m_addr_base; // Value of DW_AT_addr_base - dw_addr_t m_ranges_base; // Value of DW_AT_ranges_base - dw_offset_t m_base_obj_offset; // If this is a dwo compile unit this is the - // offset of the base compile unit in the main - // object file + lldb_private::LazyBool m_is_optimized = lldb_private::eLazyBoolCalculate; + dw_addr_t m_addr_base = 0; // Value of DW_AT_addr_base + dw_addr_t m_ranges_base = 0; // Value of DW_AT_ranges_base + // If this is a dwo compile unit this is the offset of the base compile unit + // in the main object file + dw_offset_t m_base_obj_offset = DW_INVALID_OFFSET; void ParseProducerInfo(); @@ -202,6 +202,8 @@ NameToDIE &globals, NameToDIE &types, NameToDIE &namespaces); private: + DWARFCompileUnit(SymbolFileDWARF *dwarf2Data); + const DWARFDebugInfoEntry *GetCompileUnitDIEPtrOnly() { ExtractDIEsIfNeeded(true); if (m_die_array.empty()) Index: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp === --- lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp +++ lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp @@ -39,68 +39,48 @@ extern int g_verbose; DWARFCompileUnit::DWARFCompileUnit(SymbolFileDWARF *dwarf2Data) -: m_dwarf2Data(dwarf2Data), m_abbrevs(NULL), m_user_data(NULL), - m_die_array(), m_func_aranges_ap(), m_base_addr(0), - m_offset(DW_INVALID_OFFSET), m_length(0), m_version(0), - m_addr_size(DWARFCompileUnit::GetDefaultAddressSize()), - m_producer(eProducerInvalid), m_producer_version_major(0), - m_producer_version_minor(0), m_pro
Re: [Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
I'm a little confused by your response. My stated objection to command output dependent tests is and has always been that they make the test dependent on the details of command output. Over time doing so makes it hard to modify command output. This is sort of related to interactivity, in the sense that since lldb is an interactive tool with lots of different commands producing different reports of information we can gather, the desire to improve and modify that output is more present than in tools that are less output dependent or whose output is meant to be processed, in which case it really is API. Command output for lldb is explicitly NOT API, that's why we have a real API for people who want to program lldb... So the bad effect of the tests in calcifying this output is an issue for lldb where it may not be for other tools. But interactivity per se - while it may be a bar to using the FileCheck method - has never been my objection to it. Jim > On Nov 29, 2017, at 12:18 PM, Zachary Turner wrote: > > We’ve had this discussion several times, but at the end of the day nothing > has really changed with the larger llvm project having adopted this model and > having spent significant effort in moving forward with it. > > Normally, the reason we don't use this model for LLDB tests is because they > require interactivity, and if need be I can find several threads where the > response has been, specifically "FileCheck tests aren't a good fit for LLDB > because of the interactivity and the nature of the test doesn't lend itself > well to textual output comparison". > > So here we have a case where there is no interactivity, and in fact the test > lends itself perfect to textual comparison. With all due respect, given that > the previously stated reasons for preferring not to use text-scraping tests > don't apply in this case, combined with the fact that there is a strong > desire by the larger project to use a standard, cross-project testing > infrastructure that is understood by several hundred developers instead of > several, I don't see a good reason to not at least attempt this route in > cases where it makes sense. > > > > On Wed, Nov 29, 2017 at 12:07 PM Jim Ingham via Phabricator > wrote: > jingham added a comment. > > It does look a little weird as a unit test, but to me this is mostly because > it would been much simpler to write it as a regular SB API test. > > Anyway, I really don't want the details of the text output of lldb commands > to become API. Our experience with gdb was that over time as you write more > and more tests that scrape text output, you end up not being able to change > command output because the burden of fixing up all the tests becomes too > onerous. You can use text scraping in the current lldb testsuite. We > discourage that for the reasons above, and try to isolate the tests that do > so by having lldbutils interfaces to do the explicit scraping. But it is > just as easy, and quite often much easier, to examine objects directly in the > lldb testsuite, so this mechanism encourages virtue, even though it doesn't > enforce it. > > OTOH adding a test mechanism that explicitly relies only on command output > scraping leads us down the path that ended up being a real PITN for the gdb > testsuite. So for that reason I am not in favor of going this way. > > > https://reviews.llvm.org/D40616 > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40466: DWZ 03/12: DWARFCompileUnit split to DWARFCompileUnitData
clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Take a look how the LLVM DWARF parser handles its units. It makes a DWARFUnit base class that the compile unit inherits from. That can then be used for type units and compile units. Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:23-32 +class DWARFCompileUnitDecls { public: enum Producer { eProducerInvalid = 0, eProducerClang, eProducerGCC, eProducerLLVMGCC, Just make this enum DWARFProducer and get rid of the DWARFCompileUnitDecls class. Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:34 + +class DWARFCompileUnitData : public DWARFCompileUnitDecls { +public: Remove DWARFCompileUnitDecls and just use DWARFProducer Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:34 + +class DWARFCompileUnitData : public DWARFCompileUnitDecls { +public: clayborg wrote: > Remove DWARFCompileUnitDecls and just use DWARFProducer The LLVM DWARF parser calls this class a DWARFUnit. We should probably do the same. Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:77 +class DWARFCompileUnit : public DWARFCompileUnitDecls { +public: remove DWARFCompileUnitDecls and just use DWARFProducer Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:216 + DWARFCompileUnitData *m_data; + jankratochvil wrote: > clayborg wrote: > > Is there a reason this is a member variable that I am not seeing? Seems we > > could have this class inherit from DWARFCompileUnitData. I am guessing this > > will be needed for a future patch? > Yes, future patch D40474 contains a new constructor so multiple > `DWARFCompileUnit` then point to single `DWARFCompileUnitData`. Sure that > happens only in the case ot `DW_TAG_partial_unit` (one `DWARFCompileUnit` is > read from a file while other `DWARFCompileUnit` are remapped instances with > unique offset as used from units which did use `DW_TAG_imported_unit` for > them). > `DWARFCompileUnit(DWARFCompileUnitData *data, DWARFCompileUnit *main_cu);` > We should just have an instance of this in this class and add a virtual function to retrieve it. Then we make a DWARFPartialUnit that subclasses this and has its own extra member variable and can use either one when it makes sense. Not a fan of just having a dangling pointer with no clear ownership. There is no "delete m_data" anywhere? https://reviews.llvm.org/D40466 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
On Wed, Nov 29, 2017 at 1:38 PM, Jim Ingham wrote: > I'm a little confused by your response. > > My stated objection to command output dependent tests is and has always been > that they make the test dependent on the details of command output. Over > time doing so makes it hard to modify command output. This is sort of > related to interactivity, in the sense that since lldb is an interactive tool > with lots of different commands producing different reports of information we > can gather, the desire to improve and modify that output is more present than > in tools that are less output dependent or whose output is meant to be > processed, in which case it really is API. Command output for lldb is > explicitly NOT API, that's why we have a real API for people who want to > program lldb... So the bad effect of the tests in calcifying this output is > an issue for lldb where it may not be for other tools. > Hi Jim, in my experience command output changes can be automated via `sed/grep/awk`. I'm responsible (and many others are) for fundamental changes in LLVM tools output (i.e. typeless pointers changed pretty much every load/store/memory_op* in tree) and I found out changing the output of tests isn't that troublesome. I'm not yet very familiar with LLDB, so the story might be different here. I'm personally in favour of this approach because it scaled very well in llvm (and lld, FWIW), with many more tests than lldb has. -- Davide ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40467: DWZ 04/12: Separate Offset also into FileOffset
clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Please undo all renaming from offset to file_offset. The "offset" you get from a DIE should always be the absolute .debug_info offset. No need to say file_offset. Patch will be easier to read after spurious renames are removed. Comment at: include/lldb/Expression/DWARFExpression.h:221-222 void CopyOpcodeData(lldb::ModuleSP module_sp, const DataExtractor &data, - lldb::offset_t data_offset, lldb::offset_t data_length); + lldb::offset_t data_file_offset, + lldb::offset_t data_length); don't rename, revert Comment at: source/Expression/DWARFExpression.cpp:92-94 + lldb::offset_t data_file_offset, lldb::offset_t data_length) { + const uint8_t *bytes = data.PeekData(data_file_offset, data_length); don't rename, revert Comment at: source/Plugins/SymbolFile/DWARF/DWARFAttribute.cpp:54 form_value.SetForm(FormAtIndex(i)); - lldb::offset_t offset = DIEOffsetAtIndex(i); + lldb::offset_t file_offset = cu->ThisCUUniqToFileOffset(DIEOffsetAtIndex(i)); return form_value.ExtractValue( Why is this not just done correctly inside DIEOffsetAtIndex? No need to rename the variable. Comment at: source/Plugins/SymbolFile/DWARF/DWARFAttribute.cpp:56 return form_value.ExtractValue( - cu->GetSymbolFileDWARF()->get_debug_info_data(), &offset); + cu->GetSymbolFileDWARF()->get_debug_info_data(), &file_offset); } don't rename. Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:207-226 + dw_offset_t GetFileOffset() const { return m_data->m_file_offset; } + dw_offset_t FileOffsetToUniqOffset(dw_offset_t file) const { +return ThisCUFileOffsetToUniq(file); + } + static dw_offset_t ThisCUFileOffsetToUniq_nocheck(dw_offset_t file) { +return file; + } Not a fan of these names. Can't these just be something like: ``` dw_offset_t GeCUtRelativeOffset(); // CU relative offset dw_offset_t GetOffset(); // Always absolute .debug_info offset ``` ThisCUUniqToFileOffset seems a bit unclear. https://reviews.llvm.org/D40467 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
I'm mostly basing this concern on the bad effect this had on gdb all of whose testing was expect based command scraping. gdb is a tool that's much closer to lldb than any of the compiler tools so that experience seems relevant. It's been a decade or so since I worked on gdb, but back when I was working on it, you really had to tread very carefully if you wanted to change the output of, say, the break command, or to thread listings, etc, and a bunch of times I just had to bag some cleanup of output I wanted to do because fixing up all the tests was too time consuming. Because Jason and I had both had this experience when we started working on lldb, we promised ourselves we wouldn't go down this path again... Jim > On Nov 29, 2017, at 1:43 PM, Davide Italiano wrote: > > On Wed, Nov 29, 2017 at 1:38 PM, Jim Ingham wrote: >> I'm a little confused by your response. >> >> My stated objection to command output dependent tests is and has always been >> that they make the test dependent on the details of command output. Over >> time doing so makes it hard to modify command output. This is sort of >> related to interactivity, in the sense that since lldb is an interactive >> tool with lots of different commands producing different reports of >> information we can gather, the desire to improve and modify that output is >> more present than in tools that are less output dependent or whose output is >> meant to be processed, in which case it really is API. Command output for >> lldb is explicitly NOT API, that's why we have a real API for people who >> want to program lldb... So the bad effect of the tests in calcifying this >> output is an issue for lldb where it may not be for other tools. >> > > Hi Jim, > in my experience command output changes can be automated via `sed/grep/awk`. > I'm responsible (and many others are) for fundamental changes in LLVM > tools output (i.e. typeless pointers changed pretty much every > load/store/memory_op* in tree) and I found out changing the output of > tests isn't that troublesome. I'm not yet very familiar with LLDB, so > the story might be different here. > I'm personally in favour of this approach because it scaled very well > in llvm (and lld, FWIW), with many more tests than lldb has. > > -- > Davide ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40466: DWZ 03/12: DWARFCompileUnit split to DWARFCompileUnitData
jankratochvil marked an inline comment as done. jankratochvil added inline comments. Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:216 + DWARFCompileUnitData *m_data; + clayborg wrote: > jankratochvil wrote: > > clayborg wrote: > > > Is there a reason this is a member variable that I am not seeing? Seems > > > we could have this class inherit from DWARFCompileUnitData. I am guessing > > > this will be needed for a future patch? > > Yes, future patch D40474 contains a new constructor so multiple > > `DWARFCompileUnit` then point to single `DWARFCompileUnitData`. Sure that > > happens only in the case ot `DW_TAG_partial_unit` (one `DWARFCompileUnit` > > is read from a file while other `DWARFCompileUnit` are remapped instances > > with unique offset as used from units which did use `DW_TAG_imported_unit` > > for them). > > `DWARFCompileUnit(DWARFCompileUnitData *data, DWARFCompileUnit *main_cu);` > > > We should just have an instance of this in this class and add a virtual > function to retrieve it. Then we make a DWARFPartialUnit that subclasses this > and has its own extra member variable and can use either one when it makes > sense. Not a fan of just having a dangling pointer with no clear ownership. > There is no "delete m_data" anywhere? The `DWARFCompileUnitData *m_data` content is being held in: `std::forward_list SymbolFileDWARF::m_compile_unit_data_list` as I hope/believe a `DWARFCompileUnit` is never deleted until whole `SymbolFileDWARF` is deleted. I did not use `std::shared_ptr DWARFCompileUnit::m_data` as `shared_ptr` is both memory and lock-instruction-performance expensive. https://reviews.llvm.org/D40466 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
The last time this discussion came up (Sep 2016), I pointed out that a great approach here would be to use the lldb-mi driver. The MI (Machine Interface) is a JSON-like debugger UI which closely models how people use the lldb command line driver. It was written at Cygnus (long before JSON, hence the "-like") to allow for the separation of an IDE and a debugger. All of the responses from lldb-mi are structured output; they can be ingested and queried programmatically so tests are stable. This topic has come up many times, and I know there's a real desire to say "I did these five commands & saw a problem; I just want to write a test case that does those five commands and checks for the expected output." Going from lldb command interpreter to lldb-mi command interpreter is not a big leap once you've seen how MI is structured; this may be easier for casual contributors to write. I think it would be interesting to write a test harness (based on lit?) to write tests using lldb-mi as the driver. Once people spend more time working on lldb, the SB API quickly becomes second nature so writing tests in that form is even easier. As Jim notes, we learned a very painful lesson with gdb which was based on output matching; it was fine for a while but after a couple of decades, it made it extremely difficult to change the command line tool's behavior in any way because you knew you'd be fixing ornate regular expressions that had accumulated over the years for 4x as long as it took to write the original change. It was miserable. J > On Nov 29, 2017, at 1:59 PM, Jim Ingham via lldb-commits > wrote: > > I'm mostly basing this concern on the bad effect this had on gdb all of whose > testing was expect based command scraping. gdb is a tool that's much closer > to lldb than any of the compiler tools so that experience seems relevant. > It's been a decade or so since I worked on gdb, but back when I was working > on it, you really had to tread very carefully if you wanted to change the > output of, say, the break command, or to thread listings, etc, and a bunch of > times I just had to bag some cleanup of output I wanted to do because fixing > up all the tests was too time consuming. Because Jason and I had both had > this experience when we started working on lldb, we promised ourselves we > wouldn't go down this path again... > > Jim > > >> On Nov 29, 2017, at 1:43 PM, Davide Italiano wrote: >> >> On Wed, Nov 29, 2017 at 1:38 PM, Jim Ingham wrote: >>> I'm a little confused by your response. >>> >>> My stated objection to command output dependent tests is and has always >>> been that they make the test dependent on the details of command output. >>> Over time doing so makes it hard to modify command output. This is sort of >>> related to interactivity, in the sense that since lldb is an interactive >>> tool with lots of different commands producing different reports of >>> information we can gather, the desire to improve and modify that output is >>> more present than in tools that are less output dependent or whose output >>> is meant to be processed, in which case it really is API. Command output >>> for lldb is explicitly NOT API, that's why we have a real API for people >>> who want to program lldb... So the bad effect of the tests in calcifying >>> this output is an issue for lldb where it may not be for other tools. >>> >> >> Hi Jim, >> in my experience command output changes can be automated via `sed/grep/awk`. >> I'm responsible (and many others are) for fundamental changes in LLVM >> tools output (i.e. typeless pointers changed pretty much every >> load/store/memory_op* in tree) and I found out changing the output of >> tests isn't that troublesome. I'm not yet very familiar with LLDB, so >> the story might be different here. >> I'm personally in favour of this approach because it scaled very well >> in llvm (and lld, FWIW), with many more tests than lldb has. >> >> -- >> Davide > > ___ > lldb-commits mailing list > lldb-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
> On Nov 29, 2017, at 2:10 PM, Jason Molenda via lldb-commits > wrote: > > The last time this discussion came up (Sep 2016), I pointed out that a great > approach here would be to use the lldb-mi driver. The MI (Machine Interface) > is a JSON-like debugger UI which closely models how people use the lldb > command line driver. It was written at Cygnus (long before JSON, hence the > "-like") to allow for the separation of an IDE and a debugger. All of the > responses from lldb-mi are structured output; they can be ingested and > queried programmatically so tests are stable. The MI driver is currently a text scraper itself... Many commands are not using the SB APIs like they should and are scraping text output for the response, or just making commands that will be passed down via "HandleCommand()" so I can't recommend using lldb-mi... > > This topic has come up many times, and I know there's a real desire to say "I > did these five commands & saw a problem; I just want to write a test case > that does those five commands and checks for the expected output." Going > from lldb command interpreter to lldb-mi command interpreter is not a big > leap once you've seen how MI is structured; this may be easier for casual > contributors to write. I think it would be interesting to write a test > harness (based on lit?) to write tests using lldb-mi as the driver. > > Once people spend more time working on lldb, the SB API quickly becomes > second nature so writing tests in that form is even easier. > > As Jim notes, we learned a very painful lesson with gdb which was based on > output matching; it was fine for a while but after a couple of decades, it > made it extremely difficult to change the command line tool's behavior in any > way because you knew you'd be fixing ornate regular expressions that had > accumulated over the years for 4x as long as it took to write the original > change. It was miserable. > > J > >> On Nov 29, 2017, at 1:59 PM, Jim Ingham via lldb-commits >> wrote: >> >> I'm mostly basing this concern on the bad effect this had on gdb all of >> whose testing was expect based command scraping. gdb is a tool that's much >> closer to lldb than any of the compiler tools so that experience seems >> relevant. It's been a decade or so since I worked on gdb, but back when I >> was working on it, you really had to tread very carefully if you wanted to >> change the output of, say, the break command, or to thread listings, etc, >> and a bunch of times I just had to bag some cleanup of output I wanted to do >> because fixing up all the tests was too time consuming. Because Jason and I >> had both had this experience when we started working on lldb, we promised >> ourselves we wouldn't go down this path again... >> >> Jim >> >> >>> On Nov 29, 2017, at 1:43 PM, Davide Italiano wrote: >>> >>> On Wed, Nov 29, 2017 at 1:38 PM, Jim Ingham wrote: I'm a little confused by your response. My stated objection to command output dependent tests is and has always been that they make the test dependent on the details of command output. Over time doing so makes it hard to modify command output. This is sort of related to interactivity, in the sense that since lldb is an interactive tool with lots of different commands producing different reports of information we can gather, the desire to improve and modify that output is more present than in tools that are less output dependent or whose output is meant to be processed, in which case it really is API. Command output for lldb is explicitly NOT API, that's why we have a real API for people who want to program lldb... So the bad effect of the tests in calcifying this output is an issue for lldb where it may not be for other tools. >>> >>> Hi Jim, >>> in my experience command output changes can be automated via `sed/grep/awk`. >>> I'm responsible (and many others are) for fundamental changes in LLVM >>> tools output (i.e. typeless pointers changed pretty much every >>> load/store/memory_op* in tree) and I found out changing the output of >>> tests isn't that troublesome. I'm not yet very familiar with LLDB, so >>> the story might be different here. >>> I'm personally in favour of this approach because it scaled very well >>> in llvm (and lld, FWIW), with many more tests than lldb has. >>> >>> -- >>> Davide >> >> ___ >> lldb-commits mailing list >> lldb-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits > > ___ > lldb-commits mailing list > lldb-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
> On Nov 29, 2017, at 2:13 PM, Greg Clayton wrote: > > >> On Nov 29, 2017, at 2:10 PM, Jason Molenda via lldb-commits >> wrote: >> >> The last time this discussion came up (Sep 2016), I pointed out that a great >> approach here would be to use the lldb-mi driver. The MI (Machine >> Interface) is a JSON-like debugger UI which closely models how people use >> the lldb command line driver. It was written at Cygnus (long before JSON, >> hence the "-like") to allow for the separation of an IDE and a debugger. >> All of the responses from lldb-mi are structured output; they can be >> ingested and queried programmatically so tests are stable. > > The MI driver is currently a text scraper itself... Many commands are not > using the SB APIs like they should and are scraping text output for the > response, or just making commands that will be passed down via > "HandleCommand()" so I can't recommend using lldb-mi... > That's unfortunate; but it would be easy to switch over lldb-mi to use proper SB API as the command behavior changes over time and breaks their pattern matching. Writing the tests in terms of the MI (or rather, an abstraction that represents the k-v/arrays in the MI) isolates the tests from those details. >> >> This topic has come up many times, and I know there's a real desire to say >> "I did these five commands & saw a problem; I just want to write a test case >> that does those five commands and checks for the expected output." Going >> from lldb command interpreter to lldb-mi command interpreter is not a big >> leap once you've seen how MI is structured; this may be easier for casual >> contributors to write. I think it would be interesting to write a test >> harness (based on lit?) to write tests using lldb-mi as the driver. >> >> Once people spend more time working on lldb, the SB API quickly becomes >> second nature so writing tests in that form is even easier. >> >> As Jim notes, we learned a very painful lesson with gdb which was based on >> output matching; it was fine for a while but after a couple of decades, it >> made it extremely difficult to change the command line tool's behavior in >> any way because you knew you'd be fixing ornate regular expressions that had >> accumulated over the years for 4x as long as it took to write the original >> change. It was miserable. >> >> J >> >>> On Nov 29, 2017, at 1:59 PM, Jim Ingham via lldb-commits >>> wrote: >>> >>> I'm mostly basing this concern on the bad effect this had on gdb all of >>> whose testing was expect based command scraping. gdb is a tool that's much >>> closer to lldb than any of the compiler tools so that experience seems >>> relevant. It's been a decade or so since I worked on gdb, but back when I >>> was working on it, you really had to tread very carefully if you wanted to >>> change the output of, say, the break command, or to thread listings, etc, >>> and a bunch of times I just had to bag some cleanup of output I wanted to >>> do because fixing up all the tests was too time consuming. Because Jason >>> and I had both had this experience when we started working on lldb, we >>> promised ourselves we wouldn't go down this path again... >>> >>> Jim >>> >>> On Nov 29, 2017, at 1:43 PM, Davide Italiano wrote: On Wed, Nov 29, 2017 at 1:38 PM, Jim Ingham wrote: > I'm a little confused by your response. > > My stated objection to command output dependent tests is and has always > been that they make the test dependent on the details of command output. > Over time doing so makes it hard to modify command output. This is sort > of related to interactivity, in the sense that since lldb is an > interactive tool with lots of different commands producing different > reports of information we can gather, the desire to improve and modify > that output is more present than in tools that are less output dependent > or whose output is meant to be processed, in which case it really is API. > Command output for lldb is explicitly NOT API, that's why we have a real > API for people who want to program lldb... So the bad effect of the > tests in calcifying this output is an issue for lldb where it may not be > for other tools. > Hi Jim, in my experience command output changes can be automated via `sed/grep/awk`. I'm responsible (and many others are) for fundamental changes in LLVM tools output (i.e. typeless pointers changed pretty much every load/store/memory_op* in tree) and I found out changing the output of tests isn't that troublesome. I'm not yet very familiar with LLDB, so the story might be different here. I'm personally in favour of this approach because it scaled very well in llvm (and lld, FWIW), with many more tests than lldb has. -- Davide >>> >>> ___ >>>
Re: [Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
On Wed, Nov 29, 2017 at 1:59 PM Jim Ingham wrote: > I'm mostly basing this concern on the bad effect this had on gdb all of > whose testing was expect based command scraping. gdb is a tool that's much > closer to lldb than any of the compiler tools so that experience seems > relevant. It's been a decade or so since I worked on gdb, but back when I > was working on it, you really had to tread very carefully if you wanted to > change the output of, say, the break command, or to thread listings, etc, > and a bunch of times I just had to bag some cleanup of output I wanted to > do because fixing up all the tests was too time consuming. Because Jason > and I had both had this experience when we started working on lldb, we > promised ourselves we wouldn't go down this path again... > > Couple of things: 1) I wouldn't dare to use this approach for anything that required interactivity. If you need to run one command, extract a value from the output, and use that value as input to another command, I think that would be a big mistake. I have no intention of ever proposing something like that. 2) FileCheck is very flexible in the way it matches output and tests can be written so that they are resilient to minor format tweaks. I have no doubt that with pure regex matching, or with pretty much any other tool, you would have a really bad time. Of course, that doesn't mean it would be hard to construct an example of a format change that would break a FileCheck test. But I think it would happen *far* less frequently than it did on GDB. That said, I still understand your concerns that it's fragile, so... 3) I would not be opposed to a tool called lldb-test, which was basically just LLDB with a different, and much more limited set of commands, and was completely non-interactive and would produce output in a format designed for being scraped, and which never had to be changed since it was never presented to the user. ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
> On Nov 29, 2017, at 2:51 PM, Zachary Turner via lldb-commits > wrote: > > > > On Wed, Nov 29, 2017 at 1:59 PM Jim Ingham wrote: > I'm mostly basing this concern on the bad effect this had on gdb all of whose > testing was expect based command scraping. gdb is a tool that's much closer > to lldb than any of the compiler tools so that experience seems relevant. > It's been a decade or so since I worked on gdb, but back when I was working > on it, you really had to tread very carefully if you wanted to change the > output of, say, the break command, or to thread listings, etc, and a bunch of > times I just had to bag some cleanup of output I wanted to do because fixing > up all the tests was too time consuming. Because Jason and I had both had > this experience when we started working on lldb, we promised ourselves we > wouldn't go down this path again... > > > Couple of things: > > 1) I wouldn't dare to use this approach for anything that required > interactivity. If you need to run one command, extract a value from the > output, and use that value as input to another command, I think that would be > a big mistake. I have no intention of ever proposing something like that. > > 2) FileCheck is very flexible in the way it matches output and tests can be > written so that they are resilient to minor format tweaks. I have no doubt > that with pure regex matching, or with pretty much any other tool, you would > have a really bad time. Of course, that doesn't mean it would be hard to > construct an example of a format change that would break a FileCheck test. > But I think it would happen *far* less frequently than it did on GDB. That > said, I still understand your concerns that it's fragile, so... > > 3) I would not be opposed to a tool called lldb-test, which was basically > just LLDB with a different, and much more limited set of commands, and was > completely non-interactive and would produce output in a format designed for > being scraped, and which never had to be changed since it was never presented > to the user. 100% agree with #3. We could go back and forth about using lldb-mi, but I think a specialized driver using SB API, designed for testing, would be a great approach. ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
FWIW, it can certainly use the SB API where it makes sense, but I think requiring that it only use the SB API would be very limiting and a big mistake. The entire point of a tool such as this is that it allows you to dig deep into internals that would be difficult to access otherwise. On Wed, Nov 29, 2017 at 3:23 PM Jason Molenda wrote: > > > > On Nov 29, 2017, at 2:51 PM, Zachary Turner via lldb-commits < > lldb-commits@lists.llvm.org> wrote: > > > > > > > > On Wed, Nov 29, 2017 at 1:59 PM Jim Ingham wrote: > > I'm mostly basing this concern on the bad effect this had on gdb all of > whose testing was expect based command scraping. gdb is a tool that's much > closer to lldb than any of the compiler tools so that experience seems > relevant. It's been a decade or so since I worked on gdb, but back when I > was working on it, you really had to tread very carefully if you wanted to > change the output of, say, the break command, or to thread listings, etc, > and a bunch of times I just had to bag some cleanup of output I wanted to > do because fixing up all the tests was too time consuming. Because Jason > and I had both had this experience when we started working on lldb, we > promised ourselves we wouldn't go down this path again... > > > > > > Couple of things: > > > > 1) I wouldn't dare to use this approach for anything that required > interactivity. If you need to run one command, extract a value from the > output, and use that value as input to another command, I think that would > be a big mistake. I have no intention of ever proposing something like > that. > > > > 2) FileCheck is very flexible in the way it matches output and tests can > be written so that they are resilient to minor format tweaks. I have no > doubt that with pure regex matching, or with pretty much any other tool, > you would have a really bad time. Of course, that doesn't mean it would be > hard to construct an example of a format change that would break a > FileCheck test. But I think it would happen *far* less frequently than it > did on GDB. That said, I still understand your concerns that it's fragile, > so... > > > > 3) I would not be opposed to a tool called lldb-test, which was > basically just LLDB with a different, and much more limited set of > commands, and was completely non-interactive and would produce output in a > format designed for being scraped, and which never had to be changed since > it was never presented to the user. > > > 100% agree with #3. We could go back and forth about using lldb-mi, but I > think a specialized driver using SB API, designed for testing, would be a > great approach. > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
> On Nov 29, 2017, at 3:46 PM, Zachary Turner wrote: > > FWIW, it can certainly use the SB API where it makes sense, but I think > requiring that it only use the SB API would be very limiting and a big > mistake. > > The entire point of a tool such as this is that it allows you to dig deep > into internals that would be difficult to access otherwise. I'm not sure about that. Making a test that "digs deep into internals" in this method is almost certainly going to require writing a custom command in lldb-test to poke those API's. How would this be any easier than writing a unit test? Jim > > On Wed, Nov 29, 2017 at 3:23 PM Jason Molenda wrote: > > > > On Nov 29, 2017, at 2:51 PM, Zachary Turner via lldb-commits > > wrote: > > > > > > > > On Wed, Nov 29, 2017 at 1:59 PM Jim Ingham wrote: > > I'm mostly basing this concern on the bad effect this had on gdb all of > > whose testing was expect based command scraping. gdb is a tool that's much > > closer to lldb than any of the compiler tools so that experience seems > > relevant. It's been a decade or so since I worked on gdb, but back when I > > was working on it, you really had to tread very carefully if you wanted to > > change the output of, say, the break command, or to thread listings, etc, > > and a bunch of times I just had to bag some cleanup of output I wanted to > > do because fixing up all the tests was too time consuming. Because Jason > > and I had both had this experience when we started working on lldb, we > > promised ourselves we wouldn't go down this path again... > > > > > > Couple of things: > > > > 1) I wouldn't dare to use this approach for anything that required > > interactivity. If you need to run one command, extract a value from the > > output, and use that value as input to another command, I think that would > > be a big mistake. I have no intention of ever proposing something like > > that. > > > > 2) FileCheck is very flexible in the way it matches output and tests can be > > written so that they are resilient to minor format tweaks. I have no doubt > > that with pure regex matching, or with pretty much any other tool, you > > would have a really bad time. Of course, that doesn't mean it would be > > hard to construct an example of a format change that would break a > > FileCheck test. But I think it would happen *far* less frequently than it > > did on GDB. That said, I still understand your concerns that it's fragile, > > so... > > > > 3) I would not be opposed to a tool called lldb-test, which was basically > > just LLDB with a different, and much more limited set of commands, and was > > completely non-interactive and would produce output in a format designed > > for being scraped, and which never had to be changed since it was never > > presented to the user. > > > 100% agree with #3. We could go back and forth about using lldb-mi, but I > think a specialized driver using SB API, designed for testing, would be a > great approach. > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r319389 - Fix the gtest target for the move of ArchSpecTest.cpp from Core to Utility.
Author: jingham Date: Wed Nov 29 16:23:42 2017 New Revision: 319389 URL: http://llvm.org/viewvc/llvm-project?rev=319389&view=rev Log: Fix the gtest target for the move of ArchSpecTest.cpp from Core to Utility. Modified: lldb/trunk/lldb.xcodeproj/project.pbxproj Modified: lldb/trunk/lldb.xcodeproj/project.pbxproj URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/lldb.xcodeproj/project.pbxproj?rev=319389&r1=319388&r2=319389&view=diff == --- lldb/trunk/lldb.xcodeproj/project.pbxproj (original) +++ lldb/trunk/lldb.xcodeproj/project.pbxproj Wed Nov 29 16:23:42 2017 @@ -3426,6 +3426,7 @@ 2321F9421BDD343A00BA9A93 /* Utility */ = { isa = PBXGroup; children = ( + 23E2E5161D903689006F38BB /* ArchSpecTest.cpp */, 9A3D43C81F3150D200EB767C /* ConstStringTest.cpp */, 9A3D43C71F3150D200EB767C /* LogTest.cpp */, 9A3D43CB1F3150D200EB767C /* NameMatchesTest.cpp */, @@ -3586,7 +3587,6 @@ 9A3D43E31F3237D500EB767C /* ListenerTest.cpp */, 9A3D43E21F3237D500EB767C /* StateTest.cpp */, 9A3D43E11F3237D500EB767C /* StreamCallbackTest.cpp */, - 23E2E5161D903689006F38BB /* ArchSpecTest.cpp */, 23CB14E71D66CC0E00EDDDE1 /* CMakeLists.txt */, 23CB14E61D66CC0E00EDDDE1 /* BroadcasterTest.cpp */, 23CB14E81D66CC0E00EDDDE1 /* DataExtractorTest.cpp */, ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
Many reasons. 1) a test that looks like this: ``` RUN: yaml2obj %p/Inputs/compressed-section.yaml > %t/compressed-section.obj RUN: lldb-test %s | FileCheck %s create-module -f %t/compressed-section.obj dump-module-sections compressed-section.obj ; CHECK: Section: .hello_elf ; CHECK-NEXT: Size: 9 ; CHECK-NEXT: Data: Hello ELF ; CHECK: Section: .bogus ; CHECK-NEXT: Size: 0 ``` takes about 1 minute to write, and a couple of seconds to understand, and is 12 lines of code, compared to a unit test which takes significantly longer to write and understand, and is (at least in this case) 37 lines not counting the code to implement the command 2) this is a standard test format that is understood by hundreds of developers. 3) It does not contribute to build time. 4) Any work you do to implement the `create-module` and `dump-module-sections` testing command can later be used by any other test without any additional code. With a reasonable set of commands you would reach a point where you were rarely having to update the tool. 5) Parsing text output is often the most straightforward and easiest way to verify something. Consider for example a testcommand like "dump-unwind-plan" where you could easily represent an unwind plan in text as a sequence of rules and/or heuristics. Try doing that in code, for several hundred different test cases of obscure unwind scenarios and I think you'll quickly give up and decide it's more trouble than it's worth. On Wed, Nov 29, 2017 at 4:14 PM Jim Ingham wrote: > > > > On Nov 29, 2017, at 3:46 PM, Zachary Turner wrote: > > > > FWIW, it can certainly use the SB API where it makes sense, but I think > requiring that it only use the SB API would be very limiting and a big > mistake. > > > > The entire point of a tool such as this is that it allows you to dig > deep into internals that would be difficult to access otherwise. > > I'm not sure about that. Making a test that "digs deep into internals" in > this method is almost certainly going to require writing a custom command > in lldb-test to poke those API's. How would this be any easier than > writing a unit test? > > Jim > > > > > > On Wed, Nov 29, 2017 at 3:23 PM Jason Molenda > wrote: > > > > > > > On Nov 29, 2017, at 2:51 PM, Zachary Turner via lldb-commits < > lldb-commits@lists.llvm.org> wrote: > > > > > > > > > > > > On Wed, Nov 29, 2017 at 1:59 PM Jim Ingham wrote: > > > I'm mostly basing this concern on the bad effect this had on gdb all > of whose testing was expect based command scraping. gdb is a tool that's > much closer to lldb than any of the compiler tools so that experience seems > relevant. It's been a decade or so since I worked on gdb, but back when I > was working on it, you really had to tread very carefully if you wanted to > change the output of, say, the break command, or to thread listings, etc, > and a bunch of times I just had to bag some cleanup of output I wanted to > do because fixing up all the tests was too time consuming. Because Jason > and I had both had this experience when we started working on lldb, we > promised ourselves we wouldn't go down this path again... > > > > > > > > > Couple of things: > > > > > > 1) I wouldn't dare to use this approach for anything that required > interactivity. If you need to run one command, extract a value from the > output, and use that value as input to another command, I think that would > be a big mistake. I have no intention of ever proposing something like > that. > > > > > > 2) FileCheck is very flexible in the way it matches output and tests > can be written so that they are resilient to minor format tweaks. I have > no doubt that with pure regex matching, or with pretty much any other tool, > you would have a really bad time. Of course, that doesn't mean it would be > hard to construct an example of a format change that would break a > FileCheck test. But I think it would happen *far* less frequently than it > did on GDB. That said, I still understand your concerns that it's fragile, > so... > > > > > > 3) I would not be opposed to a tool called lldb-test, which was > basically just LLDB with a different, and much more limited set of > commands, and was completely non-interactive and would produce output in a > format designed for being scraped, and which never had to be changed since > it was never presented to the user. > > > > > > 100% agree with #3. We could go back and forth about using lldb-mi, but > I think a specialized driver using SB API, designed for testing, would be a > great approach. > > > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
I thought we weren't talking about why you want this lldb-test, but rather why tests that poke deep into the lldb_private API's are or are not appropriate for your test dingus. The thing I worry about is if you start using this for very special purpose "reach deep into the lldb_private API's" type tests, then the command-set of lldb-test is not going to be a limited set of commands that are nicely composible, it is going to be a whole assortment of odd little one-off commands and trying to figure out how to use it is going to get harder and harder over time because it will be so noisy. So if I were doing this I'd impose some discipline on myself to keep that from happening, and use unit tests for anything that was really esoteric. But again, I'm unlikely to be the one that implements this so my opinions should have the weight of a kibitzer... Jim > On Nov 29, 2017, at 4:33 PM, Zachary Turner wrote: > > Many reasons. > > 1) a test that looks like this: > > ``` > RUN: yaml2obj %p/Inputs/compressed-section.yaml > %t/compressed-section.obj > RUN: lldb-test %s | FileCheck %s > > create-module -f %t/compressed-section.obj > dump-module-sections compressed-section.obj > > ; CHECK: Section: .hello_elf > ; CHECK-NEXT: Size: 9 > ; CHECK-NEXT: Data: Hello ELF > > ; CHECK: Section: .bogus > ; CHECK-NEXT: Size: 0 > ``` > > takes about 1 minute to write, and a couple of seconds to understand, and is > 12 lines of code, compared to a unit test which takes significantly longer to > write and understand, and is (at least in this case) 37 lines not counting > the code to implement the command > > 2) this is a standard test format that is understood by hundreds of > developers. > > 3) It does not contribute to build time. > > 4) Any work you do to implement the `create-module` and > `dump-module-sections` testing command can later be used by any other test > without any additional code. With a reasonable set of commands you would > reach a point where you were rarely having to update the tool. > > 5) Parsing text output is often the most straightforward and easiest way to > verify something. Consider for example a testcommand like "dump-unwind-plan" > where you could easily represent an unwind plan in text as a sequence of > rules and/or heuristics. Try doing that in code, for several hundred > different test cases of obscure unwind scenarios and I think you'll quickly > give up and decide it's more trouble than it's worth. > > On Wed, Nov 29, 2017 at 4:14 PM Jim Ingham wrote: > > > > On Nov 29, 2017, at 3:46 PM, Zachary Turner wrote: > > > > FWIW, it can certainly use the SB API where it makes sense, but I think > > requiring that it only use the SB API would be very limiting and a big > > mistake. > > > > The entire point of a tool such as this is that it allows you to dig deep > > into internals that would be difficult to access otherwise. > > I'm not sure about that. Making a test that "digs deep into internals" in > this method is almost certainly going to require writing a custom command in > lldb-test to poke those API's. How would this be any easier than writing a > unit test? > > Jim > > > > > > On Wed, Nov 29, 2017 at 3:23 PM Jason Molenda wrote: > > > > > > > On Nov 29, 2017, at 2:51 PM, Zachary Turner via lldb-commits > > > wrote: > > > > > > > > > > > > On Wed, Nov 29, 2017 at 1:59 PM Jim Ingham wrote: > > > I'm mostly basing this concern on the bad effect this had on gdb all of > > > whose testing was expect based command scraping. gdb is a tool that's > > > much closer to lldb than any of the compiler tools so that experience > > > seems relevant. It's been a decade or so since I worked on gdb, but back > > > when I was working on it, you really had to tread very carefully if you > > > wanted to change the output of, say, the break command, or to thread > > > listings, etc, and a bunch of times I just had to bag some cleanup of > > > output I wanted to do because fixing up all the tests was too time > > > consuming. Because Jason and I had both had this experience when we > > > started working on lldb, we promised ourselves we wouldn't go down this > > > path again... > > > > > > > > > Couple of things: > > > > > > 1) I wouldn't dare to use this approach for anything that required > > > interactivity. If you need to run one command, extract a value from the > > > output, and use that value as input to another command, I think that > > > would be a big mistake. I have no intention of ever proposing something > > > like that. > > > > > > 2) FileCheck is very flexible in the way it matches output and tests can > > > be written so that they are resilient to minor format tweaks. I have no > > > doubt that with pure regex matching, or with pretty much any other tool, > > > you would have a really bad time. Of course, that doesn't mean it would > > > be hard to construct an example of a format change that would break a > > > F
Re: [Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections
And of course, one such discipline would be "only use SB API's"... Jim > On Nov 29, 2017, at 5:00 PM, Jim Ingham wrote: > > I thought we weren't talking about why you want this lldb-test, but rather > why tests that poke deep into the lldb_private API's are or are not > appropriate for your test dingus. > > The thing I worry about is if you start using this for very special purpose > "reach deep into the lldb_private API's" type tests, then the command-set of > lldb-test is not going to be a limited set of commands that are nicely > composible, it is going to be a whole assortment of odd little one-off > commands and trying to figure out how to use it is going to get harder and > harder over time because it will be so noisy. So if I were doing this I'd > impose some discipline on myself to keep that from happening, and use unit > tests for anything that was really esoteric. > > But again, I'm unlikely to be the one that implements this so my opinions > should have the weight of a kibitzer... > > Jim > > >> On Nov 29, 2017, at 4:33 PM, Zachary Turner wrote: >> >> Many reasons. >> >> 1) a test that looks like this: >> >> ``` >> RUN: yaml2obj %p/Inputs/compressed-section.yaml > %t/compressed-section.obj >> RUN: lldb-test %s | FileCheck %s >> >> create-module -f %t/compressed-section.obj >> dump-module-sections compressed-section.obj >> >> ; CHECK: Section: .hello_elf >> ; CHECK-NEXT: Size: 9 >> ; CHECK-NEXT: Data: Hello ELF >> >> ; CHECK: Section: .bogus >> ; CHECK-NEXT: Size: 0 >> ``` >> >> takes about 1 minute to write, and a couple of seconds to understand, and is >> 12 lines of code, compared to a unit test which takes significantly longer >> to write and understand, and is (at least in this case) 37 lines not >> counting the code to implement the command >> >> 2) this is a standard test format that is understood by hundreds of >> developers. >> >> 3) It does not contribute to build time. >> >> 4) Any work you do to implement the `create-module` and >> `dump-module-sections` testing command can later be used by any other test >> without any additional code. With a reasonable set of commands you would >> reach a point where you were rarely having to update the tool. >> >> 5) Parsing text output is often the most straightforward and easiest way to >> verify something. Consider for example a testcommand like >> "dump-unwind-plan" where you could easily represent an unwind plan in text >> as a sequence of rules and/or heuristics. Try doing that in code, for >> several hundred different test cases of obscure unwind scenarios and I think >> you'll quickly give up and decide it's more trouble than it's worth. >> >> On Wed, Nov 29, 2017 at 4:14 PM Jim Ingham wrote: >> >> >>> On Nov 29, 2017, at 3:46 PM, Zachary Turner wrote: >>> >>> FWIW, it can certainly use the SB API where it makes sense, but I think >>> requiring that it only use the SB API would be very limiting and a big >>> mistake. >>> >>> The entire point of a tool such as this is that it allows you to dig deep >>> into internals that would be difficult to access otherwise. >> >> I'm not sure about that. Making a test that "digs deep into internals" in >> this method is almost certainly going to require writing a custom command in >> lldb-test to poke those API's. How would this be any easier than writing a >> unit test? >> >> Jim >> >> >>> >>> On Wed, Nov 29, 2017 at 3:23 PM Jason Molenda wrote: >>> >>> On Nov 29, 2017, at 2:51 PM, Zachary Turner via lldb-commits wrote: On Wed, Nov 29, 2017 at 1:59 PM Jim Ingham wrote: I'm mostly basing this concern on the bad effect this had on gdb all of whose testing was expect based command scraping. gdb is a tool that's much closer to lldb than any of the compiler tools so that experience seems relevant. It's been a decade or so since I worked on gdb, but back when I was working on it, you really had to tread very carefully if you wanted to change the output of, say, the break command, or to thread listings, etc, and a bunch of times I just had to bag some cleanup of output I wanted to do because fixing up all the tests was too time consuming. Because Jason and I had both had this experience when we started working on lldb, we promised ourselves we wouldn't go down this path again... Couple of things: 1) I wouldn't dare to use this approach for anything that required interactivity. If you need to run one command, extract a value from the output, and use that value as input to another command, I think that would be a big mistake. I have no intention of ever proposing something like that. 2) FileCheck is very flexible in the way it matches output and tests can be written so that they are resilient to minor format tweaks. I have no doubt that with pure regex matching,
[Lldb-commits] [PATCH] D40635: refactor: Simplify loop with DWARFCompileUnit::Extract
jankratochvil created this revision. Herald added a subscriber: JDevlieghere. Forgotten small simplification in https://reviews.llvm.org/D40212. https://reviews.llvm.org/D40635 Files: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp Index: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp === --- source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp +++ source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp @@ -97,12 +97,8 @@ if (m_compile_units.empty()) { if (m_dwarf2Data != NULL) { lldb::offset_t offset = 0; - for (;;) { -DWARFCompileUnitSP cu_sp = -DWARFCompileUnit::Extract(m_dwarf2Data, &offset); -if (cu_sp.get() == NULL) - break; - + DWARFCompileUnitSP cu_sp; + while ((cu_sp = DWARFCompileUnit::Extract(m_dwarf2Data, &offset))) { m_compile_units.push_back(cu_sp); offset = cu_sp->GetNextCompileUnitOffset(); Index: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp === --- source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp +++ source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp @@ -97,12 +97,8 @@ if (m_compile_units.empty()) { if (m_dwarf2Data != NULL) { lldb::offset_t offset = 0; - for (;;) { -DWARFCompileUnitSP cu_sp = -DWARFCompileUnit::Extract(m_dwarf2Data, &offset); -if (cu_sp.get() == NULL) - break; - + DWARFCompileUnitSP cu_sp; + while ((cu_sp = DWARFCompileUnit::Extract(m_dwarf2Data, &offset))) { m_compile_units.push_back(cu_sp); offset = cu_sp->GetNextCompileUnitOffset(); ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r319402 - refactor: Simplify loop with DWARFCompileUnit::Extract
Author: jankratochvil Date: Wed Nov 29 21:49:02 2017 New Revision: 319402 URL: http://llvm.org/viewvc/llvm-project?rev=319402&view=rev Log: refactor: Simplify loop with DWARFCompileUnit::Extract Forgotten small simplification in D40212. Differential revision: https://reviews.llvm.org/D40635 Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp?rev=319402&r1=319401&r2=319402&view=diff == --- lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp (original) +++ lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp Wed Nov 29 21:49:02 2017 @@ -97,12 +97,8 @@ void DWARFDebugInfo::ParseCompileUnitHea if (m_compile_units.empty()) { if (m_dwarf2Data != NULL) { lldb::offset_t offset = 0; - for (;;) { -DWARFCompileUnitSP cu_sp = -DWARFCompileUnit::Extract(m_dwarf2Data, &offset); -if (cu_sp.get() == NULL) - break; - + DWARFCompileUnitSP cu_sp; + while ((cu_sp = DWARFCompileUnit::Extract(m_dwarf2Data, &offset))) { m_compile_units.push_back(cu_sp); offset = cu_sp->GetNextCompileUnitOffset(); ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40635: refactor: Simplify loop with DWARFCompileUnit::Extract
This revision was automatically updated to reflect the committed changes. Closed by commit rL319402: refactor: Simplify loop with DWARFCompileUnit::Extract (authored by jankratochvil). Changed prior to commit: https://reviews.llvm.org/D40635?vs=124871&id=124872#toc Repository: rL LLVM https://reviews.llvm.org/D40635 Files: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp Index: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp === --- lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp +++ lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp @@ -97,12 +97,8 @@ if (m_compile_units.empty()) { if (m_dwarf2Data != NULL) { lldb::offset_t offset = 0; - for (;;) { -DWARFCompileUnitSP cu_sp = -DWARFCompileUnit::Extract(m_dwarf2Data, &offset); -if (cu_sp.get() == NULL) - break; - + DWARFCompileUnitSP cu_sp; + while ((cu_sp = DWARFCompileUnit::Extract(m_dwarf2Data, &offset))) { m_compile_units.push_back(cu_sp); offset = cu_sp->GetNextCompileUnitOffset(); Index: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp === --- lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp +++ lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp @@ -97,12 +97,8 @@ if (m_compile_units.empty()) { if (m_dwarf2Data != NULL) { lldb::offset_t offset = 0; - for (;;) { -DWARFCompileUnitSP cu_sp = -DWARFCompileUnit::Extract(m_dwarf2Data, &offset); -if (cu_sp.get() == NULL) - break; - + DWARFCompileUnitSP cu_sp; + while ((cu_sp = DWARFCompileUnit::Extract(m_dwarf2Data, &offset))) { m_compile_units.push_back(cu_sp); offset = cu_sp->GetNextCompileUnitOffset(); ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40636: Create a trivial lldb-test program
zturner created this revision. Herald added subscribers: krytarowski, mgorny, srhines. This is basically a proof-of-concept and starting point for having a testing-centric tool in LLDB. I'm sure this leaves a lot of room to be desired, but this at least allows us to have something to build on. Right now there is only one command, the `modules` command, and I created this command not because it was particularly special, but because it addressed an immediate use case and was extremely simple. Run the tool as `lldb-test module `. Getting the linker stuff working was tricky, but hopefully I did this right. Feel free to add testing related stuff to your heart's content after this goes in. Implementing the commands themselves takes some work, but once they're there they can be reused without writing any code and result in very easy to use and maintain tests. Note that due to some oddities in llvm's command line library, if you type -help you'll see a bunch of junk. I plan to investigate and hopefully fix all that in the coming https://reviews.llvm.org/D40636 Files: lldb/tools/CMakeLists.txt lldb/tools/lldb-test/CMakeLists.txt lldb/tools/lldb-test/SystemInitializerFull.cpp lldb/tools/lldb-test/SystemInitializerFull.h lldb/tools/lldb-test/lldb-test.cpp Index: lldb/tools/lldb-test/lldb-test.cpp === --- /dev/null +++ lldb/tools/lldb-test/lldb-test.cpp @@ -0,0 +1,78 @@ + +#include "SystemInitializerFull.h" +#include "lldb/Core/Debugger.h" +#include "lldb/Core/Module.h" +#include "lldb/Core/Section.h" +#include "lldb/Initialization/SystemLifetimeManager.h" +#include "lldb/Utility/DataExtractor.h" + +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/CommandLine.h" +#include "llvm/Support/ManagedStatic.h" +#include "llvm/Support/PrettyStackTrace.h" +#include "llvm/Support/Signals.h" +#include + +using namespace lldb; +using namespace lldb_private; +using namespace llvm; + +namespace opts { +cl::SubCommand ModuleSubcommand("module", "Display LLDB Module Information"); + +namespace module { +cl::list InputFilenames(cl::Positional, cl::desc(""), + cl::OneOrMore, cl::sub(ModuleSubcommand)); +} +} + +static llvm::ManagedStatic DebuggerLifetime; + +static void dumpModules(Debugger &Dbg) { + for (const auto &File : opts::module::InputFilenames) { +ModuleSpec Spec{FileSpec(File, false)}; +Spec.GetSymbolFileSpec().SetFile(File, false); + +auto ModulePtr = std::make_shared(Spec); +SectionList *Sections = ModulePtr->GetSectionList(); +if (!Sections) { + llvm::errs() << "Could not load sections for module " << File << "\n"; + continue; +} + +size_t Count = Sections->GetNumSections(0); +llvm::outs() << "Showing " << Count << " sections\n"; +for (size_t I = 0; I < Count; ++I) { + auto S = Sections->GetSectionAtIndex(I); + assert(S); + llvm::outs() << " Section " << I << "\n"; + llvm::outs() << " Name: " << S->GetName().GetStringRef() << "\n"; + llvm::outs() << " Length: " << S->GetByteSize() << "\n"; + + lldb::offset_t Offset = 0; + DataExtractor Data; + S->GetSectionData(Data); + llvm::outs() << " Data: " << Data.GetCStr(&Offset) << "\n\n"; +} + } +} + +int main(int argc, const char *argv[]) { + StringRef ToolName = argv[0]; + sys::PrintStackTraceOnErrorSignal(ToolName); + PrettyStackTraceProgram X(argc, argv); + llvm_shutdown_obj Y; + + cl::ParseCommandLineOptions(argc, argv, "LLDB Testing Utility\n"); + + DebuggerLifetime->Initialize(llvm::make_unique(), + nullptr); + + auto Dbg = lldb_private::Debugger::CreateInstance(); + + if (opts::ModuleSubcommand) +dumpModules(*Dbg); + + DebuggerLifetime->Terminate(); + return 0; +} Index: lldb/tools/lldb-test/SystemInitializerFull.h === --- /dev/null +++ lldb/tools/lldb-test/SystemInitializerFull.h @@ -0,0 +1,35 @@ +//===-- SystemInitializerFull.h -*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===--===// + +#ifndef LLDB_API_SYSTEM_INITIALIZER_FULL_H +#define LLDB_API_SYSTEM_INITIALIZER_FULL_H + +#include "lldb/Initialization/SystemInitializerCommon.h" + +namespace lldb_private { +//-- +/// Initializes lldb. +/// +/// This class is responsible for initializing all of lldb system +/// services needed to use the full LLDB application. This class is +/// not intended to be used externally, but is instead used +/// internally by SBDebugger to initialize the system. +//-- +c