[Lldb-commits] [PATCH] D65123: Restore tests for lldb-server and lldb-vscode removed at rL366590

Hui added a comment. Any chance to restore the tracking history as well? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65123/new/ https://reviews.llvm.org/D65123 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D63165: Initial support for native debugging of x86/x64 Windows processes

Hui added inline comments. Comment at: lldb/source/Plugins/Process/Utility/RegisterContextWindows_x86_64.cpp:141 +GetRegisterInfo_WoW64(const lldb_private::ArchSpec ) { + // A WoW64 register info is the same as the i386's. + std::vector _register_infos =

[Lldb-commits] [PATCH] D63165: Initial support for native debugging of x86/x64 Windows processes

Hui added inline comments. Comment at: lldb/source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_x86_64.cpp:104-109 + if (::SuspendThread(thread_handle) == -1) { +error.SetError(GetLastError(), eErrorTypeWin32); +LLDB_LOG(log, "{0} ResumeThread failed

[Lldb-commits] [PATCH] D63165: Initial support for native debugging of x86/x64 Windows processes

Hui added inline comments. Comment at: lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.h:45-49 + NativeProcessWindows(ProcessLaunchInfo _info, NativeDelegate , + llvm::Error ); + + NativeProcessWindows(lldb::pid_t pid, int terminal_fd, +

[Lldb-commits] [PATCH] D63166: Move common functionality from processwindows into processdebugger

Hui added inline comments. Comment at: source/Plugins/Process/Windows/Common/ProcessDebugger.cpp:169-170 + +Status ProcessDebugger::AttachProcess(lldb::pid_t pid, + const ProcessAttachInfo _info, +

[Lldb-commits] [PATCH] D63165: Initial support for native debugging of x86/x64 Windows processes

Hui added inline comments. Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.h:31 +class NativeProcessWindows : public NativeProcessProtocol, + public ProcessDebugger { + labath wrote: > Hui wrote: > > labath

[Lldb-commits] [PATCH] D63165: Initial support for native debugging of x86/x64 Windows processes

Hui added inline comments. Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.h:31 +class NativeProcessWindows : public NativeProcessProtocol, + public ProcessDebugger { + labath wrote: > I'm not sure this multiple

[Lldb-commits] [PATCH] D63165: Initial support for native debugging of x86/x64 Windows processes

Hui added inline comments. Comment at: source/Plugins/Process/Windows/Common/NativeRegisterContextWindows.h:31 +protected: + Status ReadAllRegisterValues(lldb::DataBufferSP _sp, + const size_t data_size); labath wrote: > Is this

[Lldb-commits] [PATCH] D63166: Move common functionality from processwindows into processdebugger

Hui added inline comments. Comment at: source/Plugins/Process/Windows/Common/ProcessDebugger.cpp:169-170 + +Status ProcessDebugger::AttachProcess(lldb::pid_t pid, + const ProcessAttachInfo _info, +

[Lldb-commits] [PATCH] D63166: Move common functionality from processwindows into processdebugger

Hui added inline comments. Comment at: source/Plugins/Process/Windows/Common/ProcessDebugger.cpp:169-170 + +Status ProcessDebugger::AttachProcess(lldb::pid_t pid, + const ProcessAttachInfo _info, +

[Lldb-commits] [PATCH] D63166: Move common functionality from processwindows into processdebugger

Hui added a comment. In D63166#1539491 , @labath wrote: > Given that you're just moving code around, this should be fine, but I am > including @amccarth, as I believe he is more familiar with this code. > > The thing I would consider in your place is

[Lldb-commits] [PATCH] D63165: Initial support for native debugging of x86/x64 Windows processes

Hui added a comment. > In D63165#1539118 , @amccarth wrote: > >> Sorry for the stupid question, but ... >> >> What exactly is meant here by "Native"? How is a NativeProcessWindows >> different from ProcessWindows? > > > The Native*** classes are meant

[Lldb-commits] [PATCH] D61687: Update Python tests for lldb-server on Windows

Hui added inline comments. Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py:208-212 +# In current implementation of llgs on Windows, as a response to '\x03' packet, the debugger +# of the native process will trigger a call

[Lldb-commits] [PATCH] D61687: Update Python tests for lldb-server on Windows

Hui added inline comments. Comment at: packages/Python/lldbsuite/test/tools/lldb-server/inferior-crash/TestGdbRemoteAbort.py:40 +@skipIfWindows # For now the signo in T* packet is always 0. @llgs_test labath wrote: > labath wrote: > > Do you have any

[Lldb-commits] [PATCH] D61687: Update Python tests for lldb-server on Windows

Hui added inline comments. Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteModuleInfo.py:24-27 +# Replace path separators in the json string either with "" or "/" on Windows. +triple = self.dbg.GetSelectedPlatform().GetTriple() +

[Lldb-commits] [PATCH] D61686: Enable lldb-server on Windows

Hui added inline comments. Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:220 +// In most cases the missing notifications do not affect lldb-server +// so we are temporarily relaxing the following for Windows. +#if !defined(_WIN32)

[Lldb-commits] [PATCH] D56229: [PECOFF] Implementation of ObjectFilePECOFF:: GetUUID()

Hui added inline comments. Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:60 + llvm::StringRef pdb_file; + if (!COFFObj->getDebugPDBInfo(pdb_info, pdb_file) && pdb_info) +return UUID::fromOptionalData(pdb_info->PDB70.Signature); labath

[Lldb-commits] [PATCH] D56237: Implement GetLoadAddress for the Windows process plugin

Hui added a comment. In D56237#1394320 , @labath wrote: > I like the direction this is going. I have one small comment about the code > organization inline, but the bigger question I have in mind is: After these > changes, is there any use for

[Lldb-commits] [PATCH] D56237: Implement GetLoadAddress for the Windows process plugin

Hui added inline comments. Comment at: source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp:82 + // Second, try through the underlying platform. + load_addr = Host::GetProcessBaseAddress(m_process->GetID()); + if (load_addr != LLDB_INVALID_ADDRESS)

[Lldb-commits] [PATCH] D56237: Implement GetLoadAddress for the Windows process plugin

Hui added a comment. In D56237#1383070 , @labath wrote: > Going to the Host straight from the DynamicLoader is the worst possible > option, since then this code will not be correct for remote processes (it > will just randomly pick up some data from the

[Lldb-commits] [PATCH] D56237: Implement GetLoadAddress for the Windows process plugin

Hui added inline comments. Comment at: source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp:82 + // Second, try through the underlying platform. + load_addr = Host::GetProcessBaseAddress(m_process->GetID()); + if (load_addr != LLDB_INVALID_ADDRESS)

[Lldb-commits] [PATCH] D56237: Implement GetFileLoadAddress for the Windows process plugin

Hui added a comment. In D56237#1344682 , @labath wrote: > I think there is something wrong here. Normally, it should be the job of the > dynamic loader plugin to figure out where a module got loaded (and populate > the target section load list).

[Lldb-commits] [PATCH] D56196: ProcessLaunchInfo: Remove Target reference

Hui added inline comments. Comment at: lldb/trunk/source/Target/ProcessLaunchInfo.cpp:220 + if (!m_pty->OpenFirstAvailableMaster(open_flags, nullptr, 0)) { +return llvm::createStringError(llvm::inconvertibleErrorCode(), +

[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

Hui added a comment. In D56230#1361746 , @Hui wrote: > It could be the llvm::sys::flattenWindowsCommandLine issue to flatten the > command “dir c:\" for Windows Command Terminal. > However it is not an issue for PS or MingGW. > > It is observed the

[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

Hui added inline comments. Comment at: include/lldb/Utility/Args.h:121 + bool GetFlattenWindowsCommandString(std::string ) const; + labath wrote: > Hui wrote: > > labath wrote: > > > Hui wrote: > > > > labath wrote: > > > > > labath wrote: > > > > > > Hui

[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

Hui added inline comments. Comment at: include/lldb/Utility/Args.h:121 + bool GetFlattenWindowsCommandString(std::string ) const; + labath wrote: > Hui wrote: > > labath wrote: > > > labath wrote: > > > > Hui wrote: > > > > > labath wrote: > > > > > > I am

[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

Hui added inline comments. Comment at: include/lldb/Utility/Args.h:121 + bool GetFlattenWindowsCommandString(std::string ) const; + labath wrote: > labath wrote: > > Hui wrote: > > > labath wrote: > > > > I am sorry for being such a pain, but I don't think

[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

Hui added inline comments. Comment at: include/lldb/Utility/Args.h:121 + bool GetFlattenWindowsCommandString(std::string ) const; + labath wrote: > I am sorry for being such a pain, but I don't think this is grammatically > correct, as `get` and `flatten`

[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

Hui added a comment. It could be the llvm::sys::flattenWindowsCommandLine issue to flatten the command “dir c:\" for Windows Command Terminal. However it is not an issue for PS or MingGW. It is observed the flattened one is "\"C:\\WINDOWS\\system32\\cmd.exe\" /C \" dir c:\" " which will

[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

Hui added a comment. In D56230#1361634 , @labath wrote: > In D56230#1358356 , @zturner wrote: > > > I've always disliked this argument and hoped that someday someone would > > remove it entirely. My recollection

[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

Hui added a comment. > - drop the `if (m_entries[i].quote)` branch. You don't need it here, and I > don't believe it will be correct anyway, because all it will do is cause > `llvm::sys::flattenWindowsCommandLine` to add one more quote level around > that and escape the quotes added by

[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

Hui added a comment. In D56230#1355248 , @labath wrote: > In D56230#1355247 , @labath wrote: > > > For example, for a `Args` vector like `lldb-server`, `gdb-remote`, > > `--log-channels=foo\\\ \\\""" '''`,

[Lldb-commits] [PATCH] D56232: [lldb-server] Add remote platform capabilities for Windows

Hui added a comment. In D56232#1349407 , @labath wrote: > In D56232#1348514 , @zturner wrote: > > > Is that even necessary? If a platform is not remote aware, `IsHost()` will > > always just return `true` by

[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

Hui added a comment. I think the key problem here is to make sure the argument will be treated as a single argument to the process launcher. To be specific to this case only, could we just provide a quote char to argument log file path and log channels on Windows? The downside is one more #if

[Lldb-commits] [PATCH] D56234: [lldb-server] Add unnamed pipe support to PipeWindows

Hui added a comment. In D56234#1352371 , @labath wrote: > Looks very nice. Thanks. There is a similar type file_t widely used in llvm. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56234/new/ https://reviews.llvm.org/D56234

[Lldb-commits] [PATCH] D56234: [lldb-server] Add unnamed pipe support to PipeWindows

Hui added inline comments. Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:1051 // and fill the data into "command_output_ptr" #if defined(__APPLE__) // Binding to port zero, we need to figure out what port it ends up My first

[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

Hui added inline comments. Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:1138 + // Double quote the string. + ::snprintf(arg_cstr, sizeof(arg_cstr), "--log-channels=\"%s\"", + env_debugserver_log_channels.c_str());

[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

Hui added inline comments. Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:1138 + // Double quote the string. + ::snprintf(arg_cstr, sizeof(arg_cstr), "--log-channels=\"%s\"", + env_debugserver_log_channels.c_str());

[Lldb-commits] [PATCH] D56229: [PECOFF] Implementation of ObjectFilePECOFF:: GetUUID()

Hui added a comment. Not quite sure but correct me if i am wrong. (1) I think the Debug Directory is optional for COFF if it does have debug information and pdb to match with. (2) The Debug Directory does not contain COFF timestamp. Using md5 seems very tentative. Please elaborate how to

[Lldb-commits] [PATCH] D56231: [lldb-server] Improve support on Windows

Hui added a comment. With all the aaron's pending reviews on lldb-server, I could try the patch with the following platform apis. Seems to me is working on Windows and no regression on Linux side. Some difference (performance) might be (1) Previous, the vfile:write packet has a maximum as

[Lldb-commits] [PATCH] D54544: Implement basic DidAttach and DidLaunch for DynamicLoaderWindowsDYLD

Hui added inline comments. Comment at: source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp:75 + +void DynamicLoaderWindowsDYLD::DidLaunch() { + Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER)); I think

[Lldb-commits] [PATCH] D53759: [PDB] Support PDB-backed expressions evaluation

Hui added inline comments. Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:819 if (!section_list) m_entry_point_address.SetOffset(offset); else This still needs to be the offset into the section. Repository: rLLDB LLDB

[Lldb-commits] [PATCH] D52618: [Windows] A basic implementation of memory allocations in a debuggee process

Hui added inline comments. Comment at: source/Plugins/Process/Windows/Common/ProcessWindows.cpp:712 +lldb::addr_t ProcessWindows::DoAllocateMemory(size_t size, uint32_t permissions, + Status ) { Looks to me they

[Lldb-commits] [PATCH] D53375: [PDB] Improve performance of the PDB DIA plugin

Hui added a comment. In https://reviews.llvm.org/D53375#1269504, @asmith wrote: > Thanks for adding the cache. We noticed the slowdown also and were discussing > the same thing. This LGTM if the other reviewers done have any comments. I think parsing types e.g. SymbolFilePDB::ParseTypes also

[Lldb-commits] [PATCH] D53094: [pecoff] Implement ObjectFilePECOFF::GetDependedModules()

Hui added inline comments. Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:810 + + std::lock_guard guard(module_sp->GetMutex()); + zturner wrote: > Does this actually need to be a `recursive_mutex`? I think there is access to the member

[Lldb-commits] [PATCH] D52461: [PDB] Introduce `PDBNameParser`

Hui added a comment. In https://reviews.llvm.org/D52461#1249259, @aleksandr.urakov wrote: > In https://reviews.llvm.org/D52461#1245058, @clayborg wrote: > > > Try to use and extend CPlusPlusLanguage::MethodName as needed. I believe it > > was recently backed by a new clang parser that knows how

[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members

Hui added inline comments. Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:696 + base_ast_type.GetOpaqueQualType(), access, + pdb_base_class->isVirtualBaseClass(), /*base_of_class*/ true); + base_classes.push_back(base_spec);

[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members

Hui added inline comments. Comment at: lit/SymbolFile/PDB/Inputs/ClassLayoutTest.cpp:37 + }; + union { // Test unnamed union. MSVC treats it as `int a; float b;` +int a; aleksandr.urakov wrote: > Here is a problem. `MicrosoftRecordLayoutBuilder` asserts

[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members

Hui added inline comments. Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:647-648 +auto member_ast_type = type_sp->GetLayoutCompilerType(); +if (!member_ast_type.IsCompleteType()) + member_ast_type.GetCompleteType(); +// CXX class

[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members

Hui added inline comments. Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:271-272 +AccessType access = TranslateMemberAccess(udt->getAccess()); +if (access == lldb::eAccessNone && udt->isNested() && +udt->getClassParent()) { + access =

[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members

Hui added inline comments. Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:246 +return ty ? ty->shared_from_this() : nullptr; + } break; case PDB_SymType::UDT: { zturner wrote: > This looks unusual. Did you clang-format it? Just tried

[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members

Hui added inline comments. Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:588 +if (result->GetID() == type_uid) { + pdb->CompleteRecordTypeForPDBSymbol(*pdb_type, result); +} aleksandr.urakov wrote: > What are the advantages and

[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members

Hui added inline comments. Comment at: source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:696 + base_ast_type.GetOpaqueQualType(), access, + pdb_base_class->isVirtualBaseClass(), /*base_of_class*/ true); + base_classes.push_back(base_spec);

[Lldb-commits] [PATCH] D49410: [PDB] Parse UDT symbols and pointers to members

Hui added inline comments. Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:587 +// only do this once. +if (result->GetID() == type_uid) { + pdb->CompleteRecordTypeForPDBSymbol(*pdb_type, result); aleksandr.urakov wrote: > I don't fully