Re: [Lldb-commits] [lldb] r321120 - Fix a couple of warnings (NFC)
On Tue, Dec 19, 2017 at 11:54 PM, Adrian Prantl via lldb-commits wrote: > Author: adrian > Date: Tue Dec 19 14:54:37 2017 > New Revision: 321120 > > URL: http://llvm.org/viewvc/llvm-project?rev=321120&view=rev > Log: > Fix a couple of warnings (NFC) > Thanks for doing this! -- Davide ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D41352: debugserver: Propagate environment in launch-mode (pr35671)
labath updated this revision to Diff 127681. labath added a comment. Herald added a subscriber: mgorny. New version: Make sure we respect variables set by --env and that they are not overridden by --forward-env (the last part relies on the fact that in the presence of multiply-defined environment variables, getenv() will pick the first one). https://reviews.llvm.org/D41352 Files: tools/debugserver/source/debugserver.cpp unittests/tools/lldb-server/CMakeLists.txt unittests/tools/lldb-server/tests/LLGSTest.cpp unittests/tools/lldb-server/tests/TestClient.cpp unittests/tools/lldb-server/tests/TestClient.h Index: unittests/tools/lldb-server/tests/TestClient.h === --- unittests/tools/lldb-server/tests/TestClient.h +++ unittests/tools/lldb-server/tests/TestClient.h @@ -21,12 +21,20 @@ #include #include +#if LLDB_SERVER_IS_DEBUGSERVER +#define LLGS_TEST(x) DISABLED_ ## x +#define DS_TEST(x) x +#else +#define LLGS_TEST(x) x +#define DS_TEST(x) DISABLED_ ## x +#endif + namespace llgs_tests { class TestClient : public lldb_private::process_gdb_remote::GDBRemoteCommunicationClient { public: - static bool IsDebugServer(); - static bool IsLldbServer(); + static bool IsDebugServer() { return LLDB_SERVER_IS_DEBUGSERVER; } + static bool IsLldbServer() { return !IsDebugServer(); } /// Launches the server, connects it to the client and returns the client. If /// Log is non-empty, the server will write it's log to this file. @@ -37,6 +45,13 @@ static llvm::Expected> launch(llvm::StringRef Log, llvm::ArrayRef InferiorArgs); + /// Allows user to pass additional arguments to the server. Be careful when + /// using this for generic tests, as the two stubs have different + /// command-line interfaces. + static llvm::Expected> + launchCustom(llvm::StringRef Log, llvm::ArrayRef ServerArgs, llvm::ArrayRef InferiorArgs); + + ~TestClient() override; llvm::Error SetInferior(llvm::ArrayRef inferior_args); llvm::Error ListThreadsInStopReply(); Index: unittests/tools/lldb-server/tests/TestClient.cpp === --- unittests/tools/lldb-server/tests/TestClient.cpp +++ unittests/tools/lldb-server/tests/TestClient.cpp @@ -27,11 +27,6 @@ using namespace llvm; namespace llgs_tests { -bool TestClient::IsDebugServer() { - return sys::path::filename(LLDB_SERVER).contains("debugserver"); -} - -bool TestClient::IsLldbServer() { return !IsDebugServer(); } TestClient::TestClient(std::unique_ptr Conn) { SetConnection(Conn.release()); @@ -56,6 +51,10 @@ } Expected> TestClient::launch(StringRef Log, ArrayRef InferiorArgs) { + return launchCustom(Log, {}, InferiorArgs); +} + +Expected> TestClient::launchCustom(StringRef Log, ArrayRef ServerArgs, ArrayRef InferiorArgs) { const ArchSpec &arch_spec = HostInfo::GetArchitecture(); Args args; args.AppendArgument(LLDB_SERVER); @@ -80,6 +79,9 @@ args.AppendArgument( ("localhost:" + Twine(listen_socket.GetLocalPortNumber())).str()); + for (StringRef arg : ServerArgs) +args.AppendArgument(arg); + if (!InferiorArgs.empty()) { args.AppendArgument("--"); for (StringRef arg : InferiorArgs) Index: unittests/tools/lldb-server/tests/LLGSTest.cpp === --- unittests/tools/lldb-server/tests/LLGSTest.cpp +++ unittests/tools/lldb-server/tests/LLGSTest.cpp @@ -16,11 +16,6 @@ using namespace llvm; TEST_F(TestBase, LaunchModePreservesEnvironment) { - if (TestClient::IsDebugServer()) { -GTEST_LOG_(WARNING) << "Test fails with debugserver: llvm.org/pr35671"; -return; - } - putenv(const_cast("LLDB_TEST_MAGIC_VARIABLE=LLDB_TEST_MAGIC_VALUE")); auto ClientOr = TestClient::launch(getLogFileName(), @@ -34,3 +29,20 @@ HasValue(testing::Property(&StopReply::getKind, WaitStatus{WaitStatus::Exit, 0}))); } + +TEST_F(TestBase, DS_TEST(DebugserverEnv)) { + // Test that --env takes precedence over inherited environment variables. + putenv(const_cast("LLDB_TEST_MAGIC_VARIABLE=foobar")); + + auto ClientOr = TestClient::launchCustom(getLogFileName(), + { "--env", "LLDB_TEST_MAGIC_VARIABLE=LLDB_TEST_MAGIC_VALUE" }, + {getInferiorPath("environment_check")}); + ASSERT_THAT_EXPECTED(ClientOr, Succeeded()); + auto &Client = **ClientOr; + + ASSERT_THAT_ERROR(Client.ContinueAll(), Succeeded()); + ASSERT_THAT_EXPECTED( + Client.GetLatestStopReplyAs(), + HasValue(testing::Property(&StopReply::getKind, + WaitStatus{WaitStatus::Exit, 0}))); +} Index: unittests/tools/lldb-server/CMakeLists.txt === --- unittests/tools/lldb-server/CMakeLists.txt +++ unittests/tools/lldb-server/CMakeLists.txt @@ -13,9 +13,9 @@ add_lldb_test_executable(environment_chec
[Lldb-commits] [PATCH] D40079: Make sure DataBufferLLVM contents are writable
labath updated this revision to Diff 127687. labath added a comment. Add the suggested MapFileData function https://reviews.llvm.org/D40079 Files: include/lldb/Interpreter/OptionValueFileSpec.h include/lldb/Symbol/ObjectFile.h include/lldb/Target/Target.h include/lldb/Utility/DataBufferLLVM.h source/Interpreter/OptionValueFileSpec.cpp source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp source/Plugins/Platform/MacOSX/PlatformDarwin.cpp source/Symbol/ObjectFile.cpp source/Target/Target.cpp source/Utility/DataBufferLLVM.cpp Index: source/Utility/DataBufferLLVM.cpp === --- source/Utility/DataBufferLLVM.cpp +++ source/Utility/DataBufferLLVM.cpp @@ -18,7 +18,8 @@ using namespace lldb_private; -DataBufferLLVM::DataBufferLLVM(std::unique_ptr MemBuffer) +DataBufferLLVM::DataBufferLLVM( +std::unique_ptr MemBuffer) : Buffer(std::move(MemBuffer)) { assert(Buffer != nullptr && "Cannot construct a DataBufferLLVM with a null buffer"); @@ -28,43 +29,40 @@ std::shared_ptr DataBufferLLVM::CreateSliceFromPath(const llvm::Twine &Path, uint64_t Size, - uint64_t Offset, bool Private) { +uint64_t Offset) { // If the file resides non-locally, pass the volatile flag so that we don't // mmap it. - if (!Private) -Private = !llvm::sys::fs::is_local(Path); + bool IsVolatile = !llvm::sys::fs::is_local(Path); - auto Buffer = llvm::MemoryBuffer::getFileSlice(Path, Size, Offset, Private); + auto Buffer = + llvm::WritableMemoryBuffer::getFileSlice(Path, Size, Offset, IsVolatile); if (!Buffer) return nullptr; return std::shared_ptr( new DataBufferLLVM(std::move(*Buffer))); } std::shared_ptr -DataBufferLLVM::CreateFromPath(const llvm::Twine &Path, bool NullTerminate, bool Private) { +DataBufferLLVM::CreateFromPath(const llvm::Twine &Path) { // If the file resides non-locally, pass the volatile flag so that we don't // mmap it. - if (!Private) -Private = !llvm::sys::fs::is_local(Path); + bool IsVolatile = !llvm::sys::fs::is_local(Path); - auto Buffer = llvm::MemoryBuffer::getFile(Path, -1, NullTerminate, Private); + auto Buffer = llvm::WritableMemoryBuffer::getFile(Path, -1, IsVolatile); if (!Buffer) return nullptr; return std::shared_ptr( new DataBufferLLVM(std::move(*Buffer))); } uint8_t *DataBufferLLVM::GetBytes() { - return const_cast(GetBuffer()); + return reinterpret_cast(Buffer->getBufferStart()); } -const uint8_t *DataBufferLLVM::GetBytes() const { return GetBuffer(); } +const uint8_t *DataBufferLLVM::GetBytes() const { + return reinterpret_cast(Buffer->getBufferStart()); +} lldb::offset_t DataBufferLLVM::GetByteSize() const { return Buffer->getBufferSize(); } - -const uint8_t *DataBufferLLVM::GetBuffer() const { - return reinterpret_cast(Buffer->getBufferStart()); -} Index: source/Target/Target.cpp === --- source/Target/Target.cpp +++ source/Target/Target.cpp @@ -2313,7 +2313,7 @@ result_valobj_sp = persistent_var_sp->GetValueObject(); execution_results = eExpressionCompleted; } else { -const char *prefix = GetExpressionPrefixContentsAsCString(); +llvm::StringRef prefix = GetExpressionPrefixContents(); Status error; execution_results = UserExpression::Evaluate(exe_ctx, options, expr, prefix, result_valobj_sp, error, @@ -4033,18 +4033,19 @@ return LanguageType(); } -const char *TargetProperties::GetExpressionPrefixContentsAsCString() { +llvm::StringRef TargetProperties::GetExpressionPrefixContents() { const uint32_t idx = ePropertyExprPrefix; OptionValueFileSpec *file = m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpec(nullptr, false, idx); if (file) { -const bool null_terminate = true; -DataBufferSP data_sp(file->GetFileContents(null_terminate)); +DataBufferSP data_sp(file->GetFileContents()); if (data_sp) - return (const char *)data_sp->GetBytes(); + return llvm::StringRef( + reinterpret_cast(data_sp->GetBytes()), + data_sp->GetByteSize()); } - return nullptr; + return ""; } bool TargetProperties::GetBreakpointsConsultPlatformAvoidList() { Index: source/Symbol/ObjectFile.cpp === --- source/Symbol/ObjectFile.cpp +++ source/Symbol/ObjectFile.cpp @@ -688,3 +688,8 @@ void ObjectFile::RelocateSection(lldb_private::Section *section) { } + +DataBufferSP ObjectFile::MapFileData(const FileSpec &file, uint64_t Size, + uint64_t Offset) { + return DataBufferLLVM::CreateSliceF
[Lldb-commits] [PATCH] D41086: [lldb] Check that a regex is valid before searching by regex for a symbol in a pdb.
zturner added inline comments. Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:394 + if (name_str.find_first_of("[]?*.-+\\") != std::string::npos) +FindTypesByRegex(RegularExpression(name_str), max_matches, types); else clayborg wrote: > You should make the RegularExpression object first and then check for errors. > If there are errors, the type lookup should happen by normal name. This is > again why I don't like us sniffing for a regular expression. If there is an > error, like when trying to lookup "char*" as mentioned by Aaron, would you > expect to see an error message saying "char*" isn't a valid regex? Then the > user thinks that the type lookup always takes a regular expression which is > not the case, just something the PDB plugin added for some reason. I do > believe part of this patch should be adding the > lldb_private::SymbolFile::FindTypesByRegex(...) to the base class and fixing > this issue by removing the regex sniffing code since it is fragile at best > (how are we doing to lookup a template type without triggering the regex > issues?). The reason I don't think it's appropriate for this patch is because nothing currently calls that method. So something much higher level would then have to be updated to call this new method, which might even entail adding a new command line option. For now, just fixing a crash is sufficient. Repository: rL LLVM https://reviews.llvm.org/D41086 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D41092: Enable more abilities in SymbolFilePDB
zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. This looks better. Technically it's possible to break this with some weird PDBs but I don't think it's possible to do better without using the native reader. Repository: rL LLVM https://reviews.llvm.org/D41092 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D41428: [lldb] This commit adds support to cache a PDB's global scope and fixes a bug in getting the source file name for a compiland
zturner added a comment. Since it seems like you're going to be doing some work in here, it would be great if you could update `lldb-test` to dump PDB symbol information. This would allow us to easily test all sorts of things in here. For example, you could find a PDB that returned an empty source file name, and then have the tool dump something like: Compiland: {foo}, Source File: {bar} then we could just grep this output against FileCheck. Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:367-370 + // For some reason, source file name could be empty, so we will walk through + // all source files of this compiland, and determine the right source file + // if any that is used to generate this compiland based on language + // indicated in compilanddetails language field. Is it DIA that is returning the empty name? Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:371 + // indicated in compilanddetails language field. + if (source_file_name.empty()) { +auto details_up = pdb_compiland->findOneChild(); Can you do an early return here? Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:382-389 +(ConstString::Compare(file_extension, ConstString("CPP"), + false) == 0 || + ConstString::Compare(file_extension, ConstString("C"), + false) == 0 || + ConstString::Compare(file_extension, ConstString("CC"), + false) == 0 || + ConstString::Compare(file_extension, ConstString("CXX"), This would probably be shorter as: ``` llvm::is_contained({"cpp", "c", "cc", "cxx"}, file_extension.GetStringRef().lower()); ``` Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:683 + + if (cu_sp) { +m_comp_units.insert(std::make_pair(id, cu_sp)); Early return here. Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:763 auto prologue = func->findOneChild(); - is_prologue = (addr == prologue->getVirtualAddress()); + if (prologue.get()) +is_prologue = (addr == prologue->getVirtualAddress()); nit: `.get()` shouldn't be necessary Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:767 auto epilogue = func->findOneChild(); - is_epilogue = (addr == epilogue->getVirtualAddress()); + if (epilogue.get()) +is_epilogue = (addr == epilogue->getVirtualAddress()); Same here. Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.h:197 std::unique_ptr m_session_up; + std::unique_ptr m_global_scope_up; uint32_t m_cached_compile_unit_count; Is this a performance win or just a convenience? I assumed the session itself would cache the global scope, but maybe that assumption was wrong. Repository: rL LLVM https://reviews.llvm.org/D41428 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D41427: [lldb] Fix crash when parsing the type of a function without any arguments
zturner added a comment. This is another example where we could test it easily if `lldb-test` could dump this. Are you willing to take a stab at this? Repository: rL LLVM https://reviews.llvm.org/D41427 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D41352: debugserver: Propagate environment in launch-mode (pr35671)
clayborg added inline comments. Comment at: tools/debugserver/source/debugserver.cpp:1426 +for (i = 0; (env_entry = host_env[i]) != NULL; ++i) + remote->Context().PushEnvironment(env_entry); + } We need to check if the env var is already in the environment in remote->Context() first before pushing the new version. I would assume that if you exec a program with an env like: ``` FOO=bar USER=me FOO=baz ``` That FOO=baz will end up being the value that is used. If the user specifies things with --env, we will just overwrite it. We might add a PushEnvironmentIfNeeded() function to the Context() class that will make sure it isn't in the list first and push it only if needed. https://reviews.llvm.org/D41352 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D40079: Make sure DataBufferLLVM contents are writable
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks good, thanks for the making the changes. This will make future tweaks to reading of file data in object files just a single change in ObjectFile.cpp. https://reviews.llvm.org/D40079 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D41086: [lldb] Check that a regex is valid before searching by regex for a symbol in a pdb.
clayborg added a comment. If you load a exe file that has a PDB file, people can currently run: (lldb) type lookup "char*" If no testing is using the regular expression stuff, then just pull it out. Do we have unit tests that depend on this working? If not, lets just pull it out from the SymbolFilePDB::FindTypes() and we can leave the fix in that uses the RegularExpression class so it won't crash in the future? Repository: rL LLVM https://reviews.llvm.org/D41086 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D41086: [lldb] Check that a regex is valid before searching by regex for a symbol in a pdb.
That seems reasonable. Seems Aaron ran into this not because he was trying to do a regex search, but because he *wasnt* trying to do a regex search. So if he doesn’t have immediate need for a regex search, and if it’s not tested anyway, removing it seems fine On Wed, Dec 20, 2017 at 10:49 AM Greg Clayton via Phabricator < revi...@reviews.llvm.org> wrote: > clayborg added a comment. > > If you load a exe file that has a PDB file, people can currently run: > > (lldb) type lookup "char*" > > If no testing is using the regular expression stuff, then just pull it > out. Do we have unit tests that depend on this working? If not, lets just > pull it out from the SymbolFilePDB::FindTypes() and we can leave the fix in > that uses the RegularExpression class so it won't crash in the future? > > > Repository: > rL LLVM > > https://reviews.llvm.org/D41086 > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D41086: [lldb] Stop searching for a symbol in a pdb by regex
asmith updated this revision to Diff 127803. asmith retitled this revision from "[lldb] Check that a regex is valid before searching by regex for a symbol in a pdb." to "[lldb] Stop searching for a symbol in a pdb by regex". asmith edited the summary of this revision. Repository: rL LLVM https://reviews.llvm.org/D41086 Files: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp source/Plugins/SymbolFile/PDB/SymbolFilePDB.h unittests/SymbolFile/PDB/SymbolFilePDBTests.cpp Index: unittests/SymbolFile/PDB/SymbolFilePDBTests.cpp === --- unittests/SymbolFile/PDB/SymbolFilePDBTests.cpp +++ unittests/SymbolFile/PDB/SymbolFilePDBTests.cpp @@ -520,6 +520,13 @@ false, 0, searched_files, results); EXPECT_GT(num_results, 1u); EXPECT_EQ(num_results, results.GetSize()); + + // We expect no exception thrown if the given regex can't be compiled + results.Clear(); + num_results = symfile->FindTypes(sc, ConstString("**"), nullptr, + false, 0, searched_files, results); + EXPECT_EQ(num_results, 0u); + EXPECT_EQ(num_results, results.GetSize()); } TEST_F(SymbolFilePDBTests, TestMaxMatches) { Index: source/Plugins/SymbolFile/PDB/SymbolFilePDB.h === --- source/Plugins/SymbolFile/PDB/SymbolFilePDB.h +++ source/Plugins/SymbolFile/PDB/SymbolFilePDB.h @@ -172,7 +172,8 @@ const llvm::pdb::PDBSymbolCompiland &cu, llvm::DenseMap &index_map) const; - void FindTypesByRegex(const std::string ®ex, uint32_t max_matches, + void FindTypesByRegex(const lldb_private::RegularExpression ®ex, +uint32_t max_matches, lldb_private::TypeMap &types); void FindTypesByName(const std::string &name, uint32_t max_matches, Index: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp === --- source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp +++ source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp @@ -19,6 +19,7 @@ #include "lldb/Symbol/ObjectFile.h" #include "lldb/Symbol/SymbolContext.h" #include "lldb/Symbol/TypeMap.h" +#include "lldb/Utility/RegularExpression.h" #include "llvm/DebugInfo/PDB/GenericError.h" #include "llvm/DebugInfo/PDB/IPDBEnumChildren.h" @@ -250,7 +251,8 @@ return nullptr; lldb::TypeSP result = pdb->CreateLLDBTypeFromPDBType(*pdb_type); - m_types.insert(std::make_pair(type_uid, result)); + if (result.get()) +m_types.insert(std::make_pair(type_uid, result)); return result.get(); } @@ -385,19 +387,16 @@ std::string name_str = name.AsCString(); - // If this might be a regex, we have to return EVERY symbol and process them - // one by one, which is going to destroy performance on large PDB files. So - // try really hard not to use a regex match. - if (name_str.find_first_of("[]?*.-+\\") != std::string::npos) -FindTypesByRegex(name_str, max_matches, types); - else -FindTypesByName(name_str, max_matches, types); + // There is an assumption 'name' is not a regex + FindTypesByName(name_str, max_matches, types); + return types.GetSize(); } -void SymbolFilePDB::FindTypesByRegex(const std::string ®ex, - uint32_t max_matches, - lldb_private::TypeMap &types) { +void +SymbolFilePDB::FindTypesByRegex(const lldb_private::RegularExpression ®ex, +uint32_t max_matches, +lldb_private::TypeMap &types) { // When searching by regex, we need to go out of our way to limit the search // space as much as possible since this searches EVERYTHING in the PDB, // manually doing regex comparisons. PDB library isn't optimized for regex @@ -409,8 +408,6 @@ auto global = m_session_up->getGlobalScope(); std::unique_ptr results; - std::regex re(regex); - uint32_t matches = 0; for (auto tag : tags_to_search) { @@ -433,7 +430,7 @@ continue; } - if (!std::regex_match(type_name, re)) + if (!regex.Execute(type_name)) continue; // This should cause the type to get cached and stored in the `m_types` Index: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp === --- source/Plugins/SymbolFile/PDB/PDBASTParser.cpp +++ source/Plugins/SymbolFile/PDB/PDBASTParser.cpp @@ -124,6 +124,8 @@ } else if (auto type_def = llvm::dyn_cast(&type)) { lldb_private::Type *target_type = m_ast.GetSymbolFile()->ResolveTypeUID(type_def->getTypeId()); +if (!target_type) + return nullptr; std::string name = type_def->getName(); uint64_t bytes = type_def->getLength(); if (!target_type) @@ -179,6 +181,8 @@ lldb_private::Type *element_type =