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

asmith created this revision. asmith added reviewers: zturner, llvm-commits. Herald added a subscriber: lldb-commits. Repository: rLLDB LLDB https://reviews.llvm.org/D56230 Files: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp Index: source/Plugins/Process/gdb-remote/GDBRemote

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

labath added a reviewer: labath. labath added a comment. Forgive my ignorance, but what makes getenv unportable? llvm uses in a bunch of places so it can't be that bad... Is it some wide string thingy? Regardless, using the lldb function for that seems fine, but if I remember correctly, each Ge

[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

zturner 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

labath requested changes to this revision. labath added inline comments. This revision now requires changes to proceed. Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:1138 + // Double quote the string. + ::snprintf(arg_cstr, sizeof(arg_cstr), "

[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

labath added a comment. > It is not that applicable for the windows process launcher to determine which > entry in the args needs to be quoted unless given very specific flag or > option. Why not? Given the argv parsing rules described here https://docs.microsoft.com/en-us/cpp/cpp/parsing-cpp-

[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] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

zturner added a comment. In D56230#1354829 , @Hui wrote: > 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 t

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

asmith updated this revision to Diff 181377. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56230/new/ https://reviews.llvm.org/D56230 Files: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp ===

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

labath added a comment. In D56230#1354829 , @Hui wrote: > I think the key problem here is to make sure the argument will be treated as > a single argument to the process launcher. Can you elaborate on that? I still don't see what is the problem with the

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

labath added a comment. In D56230#1354851 , @zturner wrote: > I almost feel like the `Args` class could be made smarter here. Because all > of the proposed solutions will still not work correctly on Linux. For > example, why is this Windows specific?

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

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

[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\\\ \\\""" '''`, `wha

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

labath added a comment. In D56230#1358102 , @Hui wrote: > What do you think of the following codes to be added in Args? > > bool Args::GetFlattenQuotedCommandString(std::string &command) const { > std::vector args_ref; > std::vector owner; > >

[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 yourse

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

labath added a comment. In D56230#1358269 , @Hui wrote: > > - 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`

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

zturner added a comment. In D56230#1358328 , @labath wrote: > In D56230#1358269 , @Hui wrote: > > > > - drop the `if (m_entries[i].quote)` branch. You don't need it here, and > > > I don't believe it will be correc

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

labath added a comment. In D56230#1358356 , @zturner wrote: > I've always disliked this argument and hoped that someday someone would > remove it entirely. My recollection (which may be wrong) is that the only > actual use of it is so that if someone ty

[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

labath added a comment. In D56230#1361650 , @Hui wrote: > +#if defined(_WIN32) > +TEST(ArgsTest, GetFlattenWindowsCommandString) { > + Args args; > + args.AppendArgument("D:\\launcher.exe"); > + args.AppendArgument("--log=abc def"); > + > + std::

[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

asmith updated this revision to Diff 182704. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56230/new/ https://reviews.llvm.org/D56230 Files: include/lldb/Utility/Args.h source/Host/windows/ProcessLauncherWindows.cpp source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp sou

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

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

[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 &command) const; + labath wrote: > I am sorry for being such a pain, but I don't think this is grammatically > correct, as `get` and `flat

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

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

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

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

[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 &command) const; + labath wrote: > labath wrote: > > Hui wrote: > > > labath wrote: > > > > I am sorry for being such a pain, but I don't t

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

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

[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 &command) const; + labath wrote: > Hui wrote: > > labath wrote: > > > labath wrote: > > > > Hui wrote: > > > > > labath wrote: > > > > > >

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

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

[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 &command) const; + labath wrote: > Hui wrote: > > labath wrote: > > > Hui wrote: > > > > labath wrote: > > > > > labath wrote: > > > > > >

[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 flatt

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

asmith updated this revision to Diff 184325. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56230/new/ https://reviews.llvm.org/D56230 Files: source/Host/windows/ProcessLauncherWindows.cpp source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp Index: source/Plugins/Process/gdb-

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

labath added a comment. In D56230#1365898 , @Hui wrote: > Hello labath, I am thinking maybe it is just because the windows command > 'dir' can't interpret "\\" in the root directory path. For example, > > This is working > "C:\\WINDOWS\\system32\\cmd.exe

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

asmith added a comment. Thanks for the feedback and interesting discussion! If no more comments, let's go with this version for now. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56230/new/ https://reviews.llvm.org/D56230 ___ lldb-commits

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

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. OK, sounds good to me then. Just please make sure the update the test decorators before committing this (I expect you'll need to remove @xfailwindows from TestQuoting and add it to the `dir c:

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

asmith updated this revision to Diff 185783. Herald added a reviewer: serge-sans-paille. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56230/new/ https://reviews.llvm.org/D56230 Files: packages/Python/lldbsuite/test/functionalities/platform/TestPlatformCommand.py packages/Python/lldb

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

asmith updated this revision to Diff 185784. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56230/new/ https://reviews.llvm.org/D56230 Files: packages/Python/lldbsuite/test/functionalities/platform/TestPlatformCommand.py packages/Python/lldbsuite/test/settings/quoting/TestQuoting.py