You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by jeking3 <gi...@git.apache.org> on 2016/11/12 21:15:20 UTC
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
GitHub user jeking3 opened a pull request:
https://github.com/apache/thrift/pull/1128
THRIFT-3873: fix compiler warnings on windows with VS2010
Also changed VERSION to THRIFT_VERSION to avoid conflicts with third party or OS headers.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jeking3/thrift defect/THRIFT-3873-take-3
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1128.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1128
----
commit a46d39010b52ab4aaae4ea1b5de477261999f1df
Author: James E. King, III <ji...@simplivity.com>
Date: 2016-07-08T22:04:21Z
THRIFT-3873: fix compiler warnings on windows with VS2010; change VERSION to THRIFT_VERSION to avoid conflicts with third party or OS headers
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r95217047
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.cpp ---
@@ -394,15 +411,19 @@ void THeaderTransport::setHeader(const string& key, const string& value) {
writeHeaders_[key] = value;
}
-size_t THeaderTransport::getMaxWriteHeadersSize() const {
+uint32_t THeaderTransport::getMaxWriteHeadersSize() const {
size_t maxWriteHeadersSize = 0;
THeaderTransport::StringToStringMap::const_iterator it;
for (it = writeHeaders_.begin(); it != writeHeaders_.end(); ++it) {
// add sizes of key and value to maxWriteHeadersSize
// 2 varints32 + the strings themselves
maxWriteHeadersSize += 5 + 5 + (it->first).length() + (it->second).length();
}
- return maxWriteHeadersSize;
+ if (maxWriteHeadersSize > std::numeric_limits<uint32_t>().max()) {
--- End diff --
I have built all third party libraries under Visual Studio 2010, 2012, 2013, and 2015 in x86 and x86 mode, in debug and release, in static and dynamic mode, where appropriate. Then I build thrift in the same configurations. I will be releasing the windows build instructions and contributed build batch files to the project so that they can be used as a reference on how to build thrift on windows for C++ with all of the features enabled, using cmake.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88791710
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.cpp ---
@@ -494,7 +519,12 @@ void THeaderTransport::flush() {
}
// Fixups after varint size calculations
- headerSize = (pkt - headerStart);
+ ptrdiff_t ptrSize = pkt - headerStart;
+ if (ptrSize > std::numeric_limits<uint32_t>().max()) {
--- End diff --
indentation
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1128: THRIFT-3873: fix compiler warnings on windows with VS201...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1128
@ben-craig Thanks these are great comments, I will make some changes to improve this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1128: THRIFT-3873: fix compiler warnings on windows with VS201...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1128
The remaining two build failures seem unrelated to these changes,. They are both failures during docker setup.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88559811
--- Diff: lib/cpp/src/thrift/transport/TServerSocket.cpp ---
@@ -60,7 +60,7 @@
#endif // _WIN32
#endif
-#if defined(_WIN32) && (_WIN32_WINNT < 0x0600)
+#if defined(_WIN32) && (_WIN32_WINNT < 0x0600) && !defined(AI_ADDRCONFIG)
#define AI_ADDRCONFIG 0x0400
--- End diff --
I'm not convinced this needs to be defined here; every windows platform windows supports has winsock2, which includes ws2def.h, which defines this and AI_PASSIVE which is used right alongside AI_ADDRCONFIG in the code... so I am going to remove the #if block here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88804996
--- Diff: lib/cpp/test/JSONProtoTest.cpp ---
@@ -262,8 +262,9 @@ BOOST_AUTO_TEST_CASE(test_json_proto_8) {
":[\"i8\",3,1,2,3]},\"13\":{\"lst\":[\"i16\",3,1,2,3]},\"14\":{\"lst\":[\"i64"
"\",3,1,2,3]}}";
+ size_t bufSiz = strlen(json_string)*sizeof(char);
--- End diff --
Fixed in the next push.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88791721
--- Diff: lib/cpp/test/JSONProtoTest.cpp ---
@@ -262,8 +262,9 @@ BOOST_AUTO_TEST_CASE(test_json_proto_8) {
":[\"i8\",3,1,2,3]},\"13\":{\"lst\":[\"i16\",3,1,2,3]},\"14\":{\"lst\":[\"i64"
"\",3,1,2,3]}}";
+ size_t bufSiz = strlen(json_string)*sizeof(char);
--- End diff --
`const std::size_t bufSize` could improve readability
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r87931893
--- Diff: lib/cpp/src/thrift/transport/TServerSocket.cpp ---
@@ -60,7 +60,7 @@
#endif // _WIN32
#endif
-#if defined(_WIN32) && (_WIN32_WINNT < 0x0600)
+#if defined(_WIN32) && (_WIN32_WINNT < 0x0600) && !defined(AI_ADDRCONFIG)
#define AI_ADDRCONFIG 0x0400
--- End diff --
You didn't break this, but this kind of thing is exactly what the platform socket header is for. Defining constants that don't belong to us often leads to trouble. Instead, we should be defining something like THRIFT_AI_ADDRCONFIG that can either be the real AI_ADDRCONFIG or 0x0400.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r95604812
--- Diff: lib/cpp/src/thrift/windows/config.h ---
@@ -36,6 +36,9 @@
#define USE_BOOST_THREAD 1
#endif
+// Something that defines PRId64 is required to build
+#define HAVE_INTTYPES_H 1
+
--- End diff --
it's probably left-over from pre-cmake times: vstudio project included in repo did not generate any config.h, yet something had to be included...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1128: THRIFT-3873: fix compiler warnings on windows with VS201...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1128
Quite the strange error in a couple of the builds. In the compilation of TNonBlockingServer.cpp it says PRIu32 is unexpected. The build is locating inttypes.h per the cmake output, so inttypes.h is being included. I write a small test program and compiled it on Ubuntu 14.04 with gcc 4.6.4 and 4.8.2 and both of them were fine in using that once inttypes.h was included.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r87931714
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.h ---
@@ -135,8 +141,9 @@ class THeaderTransport : public TVirtualTransport<THeaderTransport, TFramedTrans
void transform(uint8_t* ptr, uint32_t sz);
uint16_t getNumTransforms() const {
- int trans = writeTrans_.size();
- return trans;
+ std::vector<uint16_t>::size_type trans = writeTrans_.size();
+ assert(trans <= UINT16_MAX);
--- End diff --
If your casting error checking is just debug asserts, then you might as well stick with the casts.
My preference would be to throw an exception if the size is out of bounds.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1128: THRIFT-3873: fix compiler warnings on windows with VS201...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1128
There was just one issue on linux; I reverted a change in TBufferTransports.h (I had converted an assert to an exception, but we can figure out what to do with asserts holistically in THRIFT-3978). Hopefully this will pass. I have built this on all flavors of Visual Studio 2010, 2012, 2013, 2015 in Debug and Release, Win32 and x64, and run the unit tests. I also built this on Ubuntu 14.04. The CI build will prove out any other issues.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r95604407
--- Diff: lib/cpp/src/thrift/windows/config.h ---
@@ -36,6 +36,9 @@
#define USE_BOOST_THREAD 1
#endif
+// Something that defines PRId64 is required to build
+#define HAVE_INTTYPES_H 1
+
--- End diff --
Note I have opened THRIFT-4025 because I don't see why we have a custom config.h file on Windows when we go through the hassle of generating one with cmake.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88571093
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.h ---
@@ -135,8 +141,9 @@ class THeaderTransport : public TVirtualTransport<THeaderTransport, TFramedTrans
void transform(uint8_t* ptr, uint32_t sz);
uint16_t getNumTransforms() const {
- int trans = writeTrans_.size();
- return trans;
+ std::vector<uint16_t>::size_type trans = writeTrans_.size();
+ assert(trans <= UINT16_MAX);
--- End diff --
I added https://issues.apache.org/jira/browse/THRIFT-3978 to cover this across the codebase.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88804076
--- Diff: compiler/cpp/src/thrift/generate/t_erl_generator.cc ---
@@ -968,10 +969,13 @@ void t_erl_generator::export_string(string name, int num) {
}
void t_erl_generator::export_types_function(t_function* tfunction, string prefix) {
-
+ t_struct::members_type::size_type num = tfunction->get_arglist()->get_members().size();
+ if (num > std::numeric_limits<int>().max()) {
+ throw "integer overflow in t_erl_generator::export_types_function, name " + tfunction->get_name();
--- End diff --
Throwing std::string or char * is a pattern used throughout the compiler code. While I agree with you this is not something that should be done, I was following what was already there for continuity and maintainability.
I filed https://issues.apache.org/jira/browse/THRIFT-3982 to cover this issue across the compiler codebase. As such, I would like to request this remain.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88560045
--- Diff: compiler/cpp/CMakeLists.txt ---
@@ -18,6 +18,16 @@
#
configure_file(${CMAKE_CURRENT_SOURCE_DIR}/src/thrift/version.h.in ${CMAKE_CURRENT_BINARY_DIR}/thrift/version.h)
+if(MSVC)
+ # The winflexbison generator outputs some macros that conflict with the Visual Studio 2010 copy of stdint.h
+ # This might be fixed in later versions of Visual Studio, but an easy solution is to include stdint.h first
+ if(HAVE_STDINT_H)
+ add_definitions(/FI"stdint.h")
+ endif(HAVE_STDINT_H)
+
+ # The generated code also causes a size type warning we cannot work around easily
+ set_source_files_properties(thriftl.cc PROPERTIES COMPILE_FLAGS /wd4267)
--- End diff --
I was able to move this into thriftc.ll however we still need the /FIstdint.h to avoid compiler warnings.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88791632
--- Diff: compiler/cpp/src/thrift/generate/t_go_generator.cc ---
@@ -433,7 +434,11 @@ std::string t_go_generator::camelcase(const std::string& value) const {
if (islower(value2[i + 1])) {
value2.replace(i, 2, 1, toupper(value2[i + 1]));
}
- fix_common_initialism(value2, i);
+
+ if (i > std::numeric_limits<int>().max()) {
+ throw "integer overflow in t_go_generator::camelcase, value = " + value;
--- End diff --
looks like some indentation problem (some time ago `make format` was helping with such issues... but it might no longer work :/ )
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88791667
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.cpp ---
@@ -375,7 +383,12 @@ void THeaderTransport::resetProtocol() {
}
uint32_t THeaderTransport::getWriteBytes() {
- return wBase_ - wBuf_.get();
+ ptrdiff_t wb = wBase_ - wBuf_.get();
+ if (wb > std::numeric_limits<uint32_t>().max()) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "write size is unreasonable");
+ }
--- End diff --
as before - code repetition - some utility function would make it much cleaner (different than before, as now TTTransportException should be thrown)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88791642
--- Diff: compiler/cpp/src/thrift/thriftl.ll ---
@@ -39,11 +39,17 @@
#endif
#ifdef _MSC_VER
-//warning C4102: 'find_rule' : unreferenced label
-#pragma warning(disable:4102)
-//avoid isatty redefinition
+#pragma warning( push )
+// warning C4267: 'argument' : conversion from 'size_t' to 'int', possible loss of data
+#pragma warning( disable : 4267 )
+
+// warning C4102: 'find_rule' : unreferenced label
+#pragma warning( disable : 4102 )
+
+// avoid isatty redefinition
#define YY_NEVER_INTERACTIVE 1
+// no <unistd.h>
--- End diff --
unnecessary comment imho
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88791701
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.cpp ---
@@ -394,15 +411,19 @@ void THeaderTransport::setHeader(const string& key, const string& value) {
writeHeaders_[key] = value;
}
-size_t THeaderTransport::getMaxWriteHeadersSize() const {
+uint32_t THeaderTransport::getMaxWriteHeadersSize() const {
size_t maxWriteHeadersSize = 0;
THeaderTransport::StringToStringMap::const_iterator it;
for (it = writeHeaders_.begin(); it != writeHeaders_.end(); ++it) {
// add sizes of key and value to maxWriteHeadersSize
// 2 varints32 + the strings themselves
maxWriteHeadersSize += 5 + 5 + (it->first).length() + (it->second).length();
}
- return maxWriteHeadersSize;
+ if (maxWriteHeadersSize > std::numeric_limits<uint32_t>().max()) {
--- End diff --
I've just realized - if somebody will compile this code on 32bit machine with gcc he will get "comparison always true" warning... just a thought
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r94489293
--- Diff: compiler/cpp/src/thrift/generate/t_erl_generator.cc ---
@@ -984,10 +988,13 @@ void t_erl_generator::export_types_string(string name, int num) {
}
void t_erl_generator::export_function(t_function* tfunction, string prefix) {
-
+ t_struct::members_type::size_type num = tfunction->get_arglist()->get_members().size();
+ if (num > std::numeric_limits<int>().max()) {
--- End diff --
I'm getting a warning at ` if (num > std::numeric_limits<int>().max()) { ` in this file and also in the erlang generator (2 occurences):
`C4018: ">": Conflict between "signed" and "unsigned"`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88804934
--- Diff: compiler/cpp/src/thrift/thriftl.ll ---
@@ -39,11 +39,17 @@
#endif
#ifdef _MSC_VER
-//warning C4102: 'find_rule' : unreferenced label
-#pragma warning(disable:4102)
-//avoid isatty redefinition
+#pragma warning( push )
+// warning C4267: 'argument' : conversion from 'size_t' to 'int', possible loss of data
+#pragma warning( disable : 4267 )
+
+// warning C4102: 'find_rule' : unreferenced label
+#pragma warning( disable : 4102 )
+
+// avoid isatty redefinition
#define YY_NEVER_INTERACTIVE 1
+// no <unistd.h>
--- End diff --
Removed in next push.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r87931050
--- Diff: compiler/cpp/CMakeLists.txt ---
@@ -18,6 +18,16 @@
#
configure_file(${CMAKE_CURRENT_SOURCE_DIR}/src/thrift/version.h.in ${CMAKE_CURRENT_BINARY_DIR}/thrift/version.h)
+if(MSVC)
+ # The winflexbison generator outputs some macros that conflict with the Visual Studio 2010 copy of stdint.h
+ # This might be fixed in later versions of Visual Studio, but an easy solution is to include stdint.h first
+ if(HAVE_STDINT_H)
+ add_definitions(/FI"stdint.h")
+ endif(HAVE_STDINT_H)
+
+ # The generated code also causes a size type warning we cannot work around easily
+ set_source_files_properties(thriftl.cc PROPERTIES COMPILE_FLAGS /wd4267)
--- End diff --
This should be resolved by putting a pragma in the source code. Only put code relevant things in the build system if there are no other alternatives.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1128: THRIFT-3873: fix compiler warnings on windows with VS201...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1128
The previous CI build failed in a couple ways I cannot explain; one was in building the docker environment and the other was a failure to build something in c_glib in an autoconf build. I tried to recreate the latter locally using a cmake build environment but I could not. I rebased to the latest master and did a force push and we'll see what happens.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r95216695
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.cpp ---
@@ -375,7 +383,12 @@ void THeaderTransport::resetProtocol() {
}
uint32_t THeaderTransport::getWriteBytes() {
- return wBase_ - wBuf_.get();
+ ptrdiff_t wb = wBase_ - wBuf_.get();
+ if (wb > std::numeric_limits<uint32_t>().max()) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "write size is unreasonable");
+ }
--- End diff --
Unfortunately using a template like this caused build warnings to occur when the inputs are not the same signed-ness. I think these bounds checks need to be done with eyes wide open, and need to be specific in each case in order to avoid compiler warnings.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88805388
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.cpp ---
@@ -394,15 +411,19 @@ void THeaderTransport::setHeader(const string& key, const string& value) {
writeHeaders_[key] = value;
}
-size_t THeaderTransport::getMaxWriteHeadersSize() const {
+uint32_t THeaderTransport::getMaxWriteHeadersSize() const {
size_t maxWriteHeadersSize = 0;
THeaderTransport::StringToStringMap::const_iterator it;
for (it = writeHeaders_.begin(); it != writeHeaders_.end(); ++it) {
// add sizes of key and value to maxWriteHeadersSize
// 2 varints32 + the strings themselves
maxWriteHeadersSize += 5 + 5 + (it->first).length() + (it->second).length();
}
- return maxWriteHeadersSize;
+ if (maxWriteHeadersSize > std::numeric_limits<uint32_t>().max()) {
--- End diff --
Compiling in Win32 exposed additional warnings I will clean up. Thanks for this comment! I am also going to build with Visual Studio 2015 in 32-bit and 64-bit to make sure they are all cleaned up. I have Win64 VS2010 building without any warnings at this point. In addition I found that the tutorial CMakeLists.txt assumes ZLIB is configured so I changed it to be conditional on the ZLIB library.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88791621
--- Diff: compiler/cpp/src/thrift/generate/t_erl_generator.cc ---
@@ -968,10 +969,13 @@ void t_erl_generator::export_string(string name, int num) {
}
void t_erl_generator::export_types_function(t_function* tfunction, string prefix) {
-
+ t_struct::members_type::size_type num = tfunction->get_arglist()->get_members().size();
+ if (num > std::numeric_limits<int>().max()) {
+ throw "integer overflow in t_erl_generator::export_types_function, name " + tfunction->get_name();
--- End diff --
throwing std::string is an antipattern - it should at least be wrapped in std::runtime_error
this code is also repeated in couple of places, maybe some utility function could simplify it?
something like:
`template <typename To, typename From> To safe_numeric_cast(From i) { ... }`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88804989
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.cpp ---
@@ -375,7 +383,12 @@ void THeaderTransport::resetProtocol() {
}
uint32_t THeaderTransport::getWriteBytes() {
- return wBase_ - wBuf_.get();
+ ptrdiff_t wb = wBase_ - wBuf_.get();
+ if (wb > std::numeric_limits<uint32_t>().max()) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
+ "write size is unreasonable");
+ }
--- End diff --
I added apache::thrift::transport::safe_numeric_cast to the TTransportException header. I couldn't think of a better place to put it for now... I agree it simplifies the code in many places, and the transport code seems to be the code closest to the platform where these transform issues exist.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1128
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1128: THRIFT-3873: fix compiler warnings on windows with VS201...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1128
Hi, I pushed fixes for the open issues however I need to make sure it still builds on linux; my local build failed so it looks like there will be another push to clean that up. Thanks for your patience.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by hcorg <gi...@git.apache.org>.
Github user hcorg commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88791711
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.h ---
@@ -135,8 +142,12 @@ class THeaderTransport : public TVirtualTransport<THeaderTransport, TFramedTrans
void transform(uint8_t* ptr, uint32_t sz);
uint16_t getNumTransforms() const {
- int trans = writeTrans_.size();
- return trans;
+ std::vector<uint16_t>::size_type trans = writeTrans_.size();
+ if (trans > std::numeric_limits<uint16_t>().max()) {
+ throw TTransportException(TTransportException::CORRUPTED_DATA,
--- End diff --
indent
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r88804923
--- Diff: lib/cpp/src/thrift/transport/THeaderTransport.cpp ---
@@ -394,15 +411,19 @@ void THeaderTransport::setHeader(const string& key, const string& value) {
writeHeaders_[key] = value;
}
-size_t THeaderTransport::getMaxWriteHeadersSize() const {
+uint32_t THeaderTransport::getMaxWriteHeadersSize() const {
size_t maxWriteHeadersSize = 0;
THeaderTransport::StringToStringMap::const_iterator it;
for (it = writeHeaders_.begin(); it != writeHeaders_.end(); ++it) {
// add sizes of key and value to maxWriteHeadersSize
// 2 varints32 + the strings themselves
maxWriteHeadersSize += 5 + 5 + (it->first).length() + (it->second).length();
}
- return maxWriteHeadersSize;
+ if (maxWriteHeadersSize > std::numeric_limits<uint32_t>().max()) {
--- End diff --
I suppose the same would occur if I build Win32 instead of x64. I'll give it a try.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1128: THRIFT-3873: fix compiler warnings on windows wit...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1128#discussion_r94495800
--- Diff: compiler/cpp/src/thrift/generate/t_erl_generator.cc ---
@@ -984,10 +988,13 @@ void t_erl_generator::export_types_string(string name, int num) {
}
void t_erl_generator::export_function(t_function* tfunction, string prefix) {
-
+ t_struct::members_type::size_type num = tfunction->get_arglist()->get_members().size();
+ if (num > std::numeric_limits<int>().max()) {
--- End diff --
Yes, I just finished a suite of batch files to build zlib, openssl, libevent, and thrift for VS 2010 through 2015, win32/x64, debug/release. I started running into these signed/unsigned issues today and I am working through resolving it. The acceptance criteria for this issue is that I can build all combinations with zero warnings - we'll see how close I get.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---