This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Remove unreferenced function ResolveIPV4HostName
ClosedPublic

Authored by serge-sans-paille on Jun 24 2021, 6:24 AM.

Diff Detail

Event Timeline

serge-sans-paille requested review of this revision.Jun 24 2021, 6:24 AM
serge-sans-paille created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2021, 6:24 AM
teemperor edited reviewers, added: jasonmolenda; removed: k8stone, beanz.Jun 24 2021, 6:57 AM
teemperor added a subscriber: teemperor.

(Nit: this is from the Linux manpage but this code is only compiled on Darwin).

(Nit: this is from the Linux manpage but this code is only compiled on Darwin).

Why do you think so? I haven't tried to build it on OSX but according to GIT the last use of this function was removed by Re-landing IPv6 support for LLDB Host.

lldb/tools/debugserver/source/RNBSocket.cpp
37

`in

(Nit: this is from the Linux manpage but this code is only compiled on Darwin).

Why do you think so? I haven't tried to build it on OSX but according to GIT the last use of this function was removed by Re-landing IPv6 support for LLDB Host.

The file is part of debugserver which is Darwin exclusive and is also only compiled/used when you're on Darwin. It's also kind of its own thing: it isn't using LLVM libraries such as ADT and we usually leave it out of larger refactorings because it's kind of in "maintenance-only" mode IIRC.

Anyway, this patch is probably fine but I added Jason as a reviewer because he knows the debugserver best.

The file is part of debugserver which is Darwin exclusive and is also only compiled/used when you're on Darwin. It's also kind of its own thing: it isn't using LLVM libraries such as ADT and we usually leave it out of larger refactorings because it's kind of in "maintenance-only" mode IIRC.

I still do not understand what code can use this function on OSX and on OSX I build with -DLLDB_USE_SYSTEM_DEBUGSERVER=ON which probably won't build the new lldb-server then.

Besides that IMO @serge-sans-paille may not need to patch it when it is OSX-only function.

lldb/tools/debugserver/source/RNBSocket.cpp
37

Here the prototype does not permit IPv6.

40

This is redundant with getaddrinfo.

48

This is redundant with getaddrinfo.

56

Here it should use ((sockaddr_in *)addr->ai_addr)->sin_addr after verifying it is an IPv4 address - instead of the inet_ntoa+inet_pton calls.
Also it should iterate all the addresses by addr->ai_next (which is not possible with the ResolveIPV4HostName function prototype).

The file is part of debugserver which is Darwin exclusive and is also only compiled/used when you're on Darwin. It's also kind of its own thing: it isn't using LLVM libraries such as ADT and we usually leave it out of larger refactorings because it's kind of in "maintenance-only" mode IIRC.

I still do not understand what code can use this function on OSX and on OSX I build with -DLLDB_USE_SYSTEM_DEBUGSERVER=ON which probably won't build the new lldb-server then.

We build both lldb-server and debugserver on Darwin but we're only using debugserver by default there. And LLDB_USE_SYSTEM_DEBUGSERVER just makes us copy the (codesigned) system debugserver into our build directory instead of building our own (but we still build lldb-server with that option IIRC).

jankratochvil added inline comments.Jun 24 2021, 7:54 AM
lldb/tools/debugserver/source/RNBSocket.cpp
44

Here it should use AI_PASSIVE instead.

On OSX with -DLLDB_USE_SYSTEM_DEBUGSERVER=OFF I have built:
-rwxr-xr-x 1 macbook staff 689200 Jun 24 19:10 bin/debugserver
It uses lldb/tools/debugserver/source/RNBSocket.cpp (if I mess up the source file it does not build). The build does not need this ResolveIPV4HostName function.
So unless there is some more OSX magic the function can be removed.

On OSX with -DLLDB_USE_SYSTEM_DEBUGSERVER=OFF I have built:
-rwxr-xr-x 1 macbook staff 689200 Jun 24 19:10 bin/debugserver
It uses lldb/tools/debugserver/source/RNBSocket.cpp (if I mess up the source file it does not build). The build does not need this ResolveIPV4HostName function.
So unless there is some more OSX magic the function can be removed.

Sorry, I thought you meant with "this function" gethostbyname not ResolveIPV4HostName. You're right, this is indeed unused and it's also used in our downstream repo I think, so let's get rid of it. Good catch!

jankratochvil requested changes to this revision.Jun 24 2021, 11:41 PM
This revision now requires changes to proceed.Jun 24 2021, 11:41 PM
serge-sans-paille retitled this revision from [lldb] replace gethostbyname call by getaddrinfo to [NFC] Remove unreferenced function ResolveIPV4HostName.
serge-sans-paille edited the summary of this revision. (Show Details)

Just remove the function

What a simpler patch!

jankratochvil accepted this revision.Jun 25 2021, 1:16 AM
This revision is now accepted and ready to land.Jun 25 2021, 1:16 AM