Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12643 )
Change subject: [thirdparty] Bump RapidJSON version to 1.1.0
......................................................................
Patch Set 5:
(6 comments)
> looks like lots of test failures from this new version in ASAN builds. I
> think Mike and Adar have been looking at rapidjson UBSAN problems as well
I pulled out one of the failures below. Like Todd said, let us know if you need
help debugging this.
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/installed/common/include/rapidjson/internal/dtoa.h:88:16:
runtime error: unsigned integer overflow: 4294967292 + 4 cannot be represented
in type 'unsigned int'
#0 0x7f3b9b8dabd1 in
rapidjson::internal::DigitGen(rapidjson::internal::DiyFp const&,
rapidjson::internal::DiyFp const&, unsigned long, char*, int*, int*)
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/installed/common/include/rapidjson/internal/dtoa.h
#1 0x7f3b9b8d8638 in rapidjson::internal::Grisu2(double, char*, int*, int*)
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/installed/common/include/rapidjson/internal/dtoa.h:123:5
#2 0x7f3b9b8d8039 in rapidjson::internal::dtoa(double, char*, int)
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/installed/common/include/rapidjson/internal/dtoa.h:233:9
#3 0x7f3b9b982f0c in rapidjson::Writer<kudu::UTF8StringStreamBuffer,
rapidjson::UTF8<char>, rapidjson::UTF8<char>, rapidjson::CrtAllocator,
0u>::WriteDouble(double)
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/installed/common/include/rapidjson/writer.h:338:21
#4 0x7f3b9b97da93 in
kudu::JsonWriter::ProtobufField(google::protobuf::Message const&,
google::protobuf::Reflection const*, google::protobuf::FieldDescriptor const*)
/home/jenkins-slave/workspace/kudu-master/1/src/kudu/util/jsonwriter.cc
#5 0x7f3b9b97c472 in kudu::JsonWriter::Protobuf(google::protobuf::Message
const&)
/home/jenkins-slave/workspace/kudu-master/1/src/kudu/util/jsonwriter.cc:214:7
#6 0x7f3b9b9e54a3 in kudu::Histogram::WriteAsJson(kudu::JsonWriter*,
kudu::MetricJsonOptions const&) const
/home/jenkins-slave/workspace/kudu-master/1/src/kudu/util/metrics.cc:658:11
#7 0x7f3b9b9df851 in kudu::MetricEntity::WriteAsJson(kudu::JsonWriter*,
std::vector<std::string, std::allocator<std::string> > const&,
kudu::MetricJsonOptions const&) const
/home/jenkins-slave/workspace/kudu-master/1/src/kudu/util/metrics.cc:264:7
#8 0x7f3b9b9e18fc in kudu::MetricRegistry::WriteAsJson(kudu::JsonWriter*,
std::vector<std::string, std::allocator<std::string> > const&,
kudu::MetricJsonOptions const&) const
/home/jenkins-slave/workspace/kudu-master/1/src/kudu/util/metrics.cc:356:5
#9 0x7f3ba6efd4c6 in kudu::WriteMetricsAsJson(kudu::MetricRegistry const*,
kudu::WebCallbackRegistry::WebRequest const&,
kudu::WebCallbackRegistry::PrerenderedWebResponse*)
/home/jenkins-slave/workspace/kudu-master/1/src/kudu/server/default_path_handlers.cc:359:3
#10 0x7f3ba6f07f41 in void boost::_bi::bind_t<void, void
(*)(kudu::MetricRegistry const*, kudu::WebCallbackRegistry::WebRequest const&,
kudu::WebCallbackRegistry::PrerenderedWebResponse*),
boost::_bi::list3<boost::_bi::value<kudu::MetricRegistry const*>,
boost::arg<1>, boost::arg<2> >
>::operator()<kudu::WebCallbackRegistry::WebRequest const&,
kudu::WebCallbackRegistry::PrerenderedWebResponse*>(kudu::WebCallbackRegistry::WebRequest
const&, kudu::WebCallbackRegistry::PrerenderedWebResponse*&&)
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/installed/uninstrumented/include/boost/bind/bind.hpp:1246:16
#11 0x7f3ba6f07aef in
boost::detail::function::void_function_obj_invoker2<boost::_bi::bind_t<void,
void (*)(kudu::MetricRegistry const*, kudu::WebCallbackRegistry::WebRequest
const&, kudu::WebCallbackRegistry::PrerenderedWebResponse*),
boost::_bi::list3<boost::_bi::value<kudu::MetricRegistry const*>,
boost::arg<1>, boost::arg<2> > >, void, kudu::WebCallbackRegistry::WebRequest
const&,
kudu::WebCallbackRegistry::PrerenderedWebResponse*>::invoke(boost::detail::function::function_buffer&,
kudu::WebCallbackRegistry::WebRequest const&,
kudu::WebCallbackRegistry::PrerenderedWebResponse*)
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/installed/uninstrumented/include/boost/function/function_template.hpp:159:11
#12 0x7f3ba6f86a19 in boost::function2<void,
kudu::WebCallbackRegistry::WebRequest const&,
kudu::WebCallbackRegistry::PrerenderedWebResponse*>::operator()(kudu::WebCallbackRegistry::WebRequest
const&, kudu::WebCallbackRegistry::PrerenderedWebResponse*) const
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/installed/uninstrumented/include/boost/function/function_template.hpp:770:14
#13 0x7f3ba6f7e082 in
kudu::Webserver::RunPathHandler(kudu::Webserver::PathHandler const&,
sq_connection*, sq_request_info*)
/home/jenkins-slave/workspace/kudu-master/1/src/kudu/server/webserver.cc:469:5
#14 0x7f3ba6f7cd86 in kudu::Webserver::BeginRequestCallback(sq_connection*,
sq_request_info*)
/home/jenkins-slave/workspace/kudu-master/1/src/kudu/server/webserver.cc:403:10
#15 0x7f3ba6fad436 in handle_request
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/src/squeasel-9335b81317a6451d5a37c5dc7ec088eecbf68c82/squeasel.c:3865
#16 0x7f3ba6fafbb2 in process_new_connection
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/src/squeasel-9335b81317a6451d5a37c5dc7ec088eecbf68c82/squeasel.c:4496
#17 0x7f3ba6fb03d7 in worker_thread
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/src/squeasel-9335b81317a6451d5a37c5dc7ec088eecbf68c82/squeasel.c:4628
#18 0x7f3ba1bee183 in start_thread
/build/eglibc-SvCtMH/eglibc-2.19/nptl/pthread_create.c:312
#19 0x7f3b96f45ffc in clone sysdeps/unix/sysv/linux/x86_64/clone.S:111
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/installed/common/include/rapidjson/internal/dtoa.h:88:16
in
http://gerrit.cloudera.org:8080/#/c/12643/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12643/5//COMMIT_MSG@16
PS5, Line 16: output multiple JSONs, and also add an unit test for this.
> Yes, and an unit test TestTablet.TestMetricsInit in src/kudu/tablet/tablet-
If the only usage is test-only, It'd be simpler to just create a new JsonWriter
and be done with it. Is there a production use case for Reset()?
http://gerrit.cloudera.org:8080/#/c/12643/5/src/kudu/util/jsonreader-test.cc
File src/kudu/util/jsonreader-test.cc:
http://gerrit.cloudera.org:8080/#/c/12643/5/src/kudu/util/jsonreader-test.cc@147
PS5, Line 147: // The rapidjson code has some improper handling of the min
int32 and min
: // int64 that exposes UB.
: #if defined(ADDRESS_SANITIZER)
: LOG(WARNING) << "this test is skipped in ASAN builds";
: return;
: #endif
We should be able to remove this now.
http://gerrit.cloudera.org:8080/#/c/12643/5/src/kudu/util/jsonwriter-test.cc
File src/kudu/util/jsonwriter-test.cc:
http://gerrit.cloudera.org:8080/#/c/12643/5/src/kudu/util/jsonwriter-test.cc@186
PS5, Line 186: void TestJsonWriter::DoBenchmark(const Message& pb) {
Could you run these benchmarks before and after the upgrade in RELEASE mode? Is
there any difference?
http://gerrit.cloudera.org:8080/#/c/12643/5/src/kudu/util/jsonwriter.h
File src/kudu/util/jsonwriter.h:
http://gerrit.cloudera.org:8080/#/c/12643/5/src/kudu/util/jsonwriter.h@83
PS5, Line 83: void Reset();
Could you document what this does in a comment?
http://gerrit.cloudera.org:8080/#/c/12643/5/src/kudu/util/jsonwriter.cc
File src/kudu/util/jsonwriter.cc:
http://gerrit.cloudera.org:8080/#/c/12643/5/src/kudu/util/jsonwriter.cc@30
PS5, Line 30: #include <rapidjson/internal/../rapidjson.h>
What was wrong with the existing inclusion of rapidjson/rapidjson.h?
http://gerrit.cloudera.org:8080/#/c/12643/5/src/kudu/util/jsonwriter.cc@378
PS5, Line 378: }
Nit: add an empty line after this one.
--
To view, visit http://gerrit.cloudera.org:8080/12643
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c5e6bf6b8c6802bde28716487abb2b242a7a578
Gerrit-Change-Number: 12643
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <[email protected]>
Gerrit-Reviewer: Yingchun Lai <[email protected]>
Gerrit-Comment-Date: Tue, 05 Mar 2019 00:32:11 +0000
Gerrit-HasComments: Yes
