rupprecht added a comment. In D87333#2274572 <https://reviews.llvm.org/D87333#2274572>, @rupprecht wrote:
> In D87333#2273506 <https://reviews.llvm.org/D87333#2273506>, @labath wrote: > >> (Sorry about the delay.) Given the current requirements, I think this patch >> is fine (excellent even). >> >> That said, I'm not sure whether the original motivation for this requirement >> (avoiding dns lookups) is still relevant. These days, we communicate with >> the local debug server via `socketpair(2)` sockets (which wasn't the case >> back then), which does not require any dns lookups and is immune to a >> misconfigured hosts file. > > The initial dns lookup may still fail, I think? e.g. my initial version of > this patch had: > > if (llvm::Error error = > listen_socket.Listen("localhost:0", backlog).ToError()) > > If the hosts file has a bad value for localhost, that will create a bad > socket. That said, I don't know how to reproduce the original issue -- maybe > it isn't an issue for some other reason. > >> Given that none of the things touched by this patch is extremely critical >> (`GDBRemoteCommunication::ConnectLocally` is used for reproducers and the >> rest is test code), maybe we could use this to test the water and see >> whether we can start using the network stack the way it's supposed to be >> used ? > > Many non-reproducers tests fail w/o the change to > `GDBRemoteCommunication::ConnectLocally`, so it's at least used outside of > that. > > I can go either way with using `localhost` or using hard-coded localhost IPs. > Whichever way we go, I have a slight preference for keeping the tests and > code consistent, e.g. if we use hard-coded IPs in code linked into lldb to > allow people with a bad hosts file to use lldb, we should use hard-coded IPs > in the tests to allow those same users to develop on lldb. Switched back to the original approach of using `localhost` when possible [1], though I still have the git branch so I can always switch back. I don't have a personal preference either way, but would like to land one of the two approaches as this creates a strange amount of test flakiness internally when we need to support ipv4. [1] IIUC, when connecting via hostname, attempting "localhost" will try both AF_INET and AF_INET6, but there is no corresponding way to construct a raw socket with automatic fallback to AF_INET6 if AF_INET does not work, so that part is the same as before (try to create AF_INET and catch the error manually) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87333/new/ https://reviews.llvm.org/D87333 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
