Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect call to Unicode Win32 InetPton #4306

Merged
merged 2 commits into from Nov 12, 2019

Conversation

jmorrill
Copy link
Contributor

The recent commit for #4281 is calling the Win32 MACRO InetPton. If the Visual studio project is set to "Use Unicode Character Set", (which tvm.dll seems to be) this calls InetPtonW, which uses wchar_t* (aka PCWSTR). This causes a build error as the "addr_buf" is a char*.

From InetPton docs:

When UNICODE or _UNICODE is defined, InetPton is defined to InetPtonW, the Unicode version of this function. The pszAddrString parameter is defined to the PCWSTR data type.

When UNICODE or _UNICODE is not defined, InetPton is defined to InetPtonA, the ANSI version of this function. The ANSI version of this function is always defined as inet_pton. The pszAddrString parameter is defined to the PCSTR data type.

This can be fixed by directly calling InetPtonA, which is safe as the tvm function always uses a char* for add_str and InetPtonA always takes a char* (aka PCSTR)

Copy link
Contributor

@soiferj soiferj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@FrozenGene FrozenGene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM.

@FrozenGene
Copy link
Member

FrozenGene commented Nov 11, 2019

@jmorrill I have read the doc: https://docs.microsoft.com/en-us/windows/win32/api/ws2tcpip/nf-ws2tcpip-inetptonw. Seems that Windows has Unix's inet_pton function too. i.e. InetPtonA defines. Do you think we should be better to use inet_pton directly and needn't distinguish _WIN32 / Unix?

i.e.

inline bool ValidateIP(std::string ip) {
  if (ip == "localhost") {
    return true;
  }
  struct sockaddr_in sa_ipv4;
  struct sockaddr_in6 sa_ipv6;
  bool is_ipv4 = inet_pton(AF_INET, ip.c_str(), &(sa_ipv4.sin_addr));
  bool is_ipv6 = inet_pton(AF_INET6, ip.c_str(), &(sa_ipv6.sin6_addr));
  return is_ipv4 || is_ipv6;
}

And I am curious why we don't function redefinition error because Windows has inet_pton too.

@@ -59,7 +59,7 @@ static inline int poll(struct pollfd *pfd, int nfds,
return WSAPoll(pfd, nfds, timeout);
}
static inline int inet_pton(int family, const char* addr_str, void* addr_buf) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe be better to remove? Because WIndows / Unix both have inet_pton as comment said before

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find! I removed the "static inline int inet_pton" function from socket.h and got a successful build on Windows.

Should I close this PR and resubmit a new PR with it removed?

@tqchen
Copy link
Member

tqchen commented Nov 12, 2019

@jmorrill seems there was some problem in rebasing, please cherry pick your windows change and rebase against the master

@jmorrill
Copy link
Contributor Author

jmorrill commented Nov 12, 2019

Sorry you are dealing with a n00b, I'm still new with these git features.

The commit we want is here: 6f96747

git fetch upstream
git rebase upstream/master

Reports everything up to date

@FrozenGene
Copy link
Member

Sorry you are dealing with a n00b, I'm still new with these git features.

The commit we want is here: 6f96747

git fetch upstream
git rebase upstream/master

Reports everything up to date

No worries. Our TVM website has one good Git usage tips: https://docs.tvm.ai/contribute/git_howto.html

Copy link
Member

@FrozenGene FrozenGene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@tqchen tqchen merged commit 2571449 into apache:master Nov 12, 2019
@tqchen
Copy link
Member

tqchen commented Nov 12, 2019

Thanks @jmorrill @FrozenGene @soiferj !

zxy844288792 pushed a commit to neo-ai/tvm that referenced this pull request Nov 13, 2019
* Fix incorrect call to Unicode Win32

* Removed inet_pton call. Win32 already has it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants