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.

Sign up for GitHub

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

Jump to bottom

THRIFT-4134-Fix-remaining-undefined-behavior-invalid.patch #1222

Closed
wants to merge 3 commits into from
Closed

THRIFT-4134-Fix-remaining-undefined-behavior-invalid.patch #1222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4dca9e6
THRIFT-4134-Fix-remaining-undefined-behavior-invalid.patch
Jens-G Mar 26, 2017
0d3cc72
THRIFT-4134-Fix-remaining-undefined-behavior-invalid.patch
Jens-G Mar 26, 2017
e67ee35
THRIFT-4134 Fix remaining undefined behavior invalid vptr
Jens-G Mar 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build/docker/scripts/ubsan.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export CXX=clang++-3.8
# undefined casting, aka "vptr".
#
# TODO: fix undefined vptr behavior and turn this option back on.
export CFLAGS="-fsanitize=undefined -fno-sanitize-recover=undefined -fno-sanitize=vptr"
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll still want to include -fno-sanitize-recover=undefined, I think. When that option is set, a binary will crash on undefined behavior. When it is not set, the binary will print an error and continue.

Having it set is preferable, I think, so that undefined behavior causes the tests to fail.

Copy link
Member Author

@Jens-G Jens-G Mar 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I just used your proposal. Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

I should have been more explicit. For finding vptr errors, it's good to remove -fno-sanitize-recover=undefined. But for checking that there are none, it's good to have -fno-sanitize-recover=undefined.

If this patch fixes some, but not all, of the vptr errors, I would suggest leaving CFLAGS as it is in master. If it fixes all vptr errors, I think the way you have it now is optimal.

export CFLAGS="-fsanitize=undefined -fno-sanitize-recover=undefined"
# Builds without optimization and with debugging symbols for making crash reports more
# readable.
export CFLAGS="${CFLAGS} -O0 -ggdb3"
Expand Down
2 changes: 1 addition & 1 deletion compiler/cpp/src/thrift/generate/t_haxe_generator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2573,7 +2573,7 @@ string t_haxe_generator::type_name(t_type* ttype, bool in_container, bool in_ini
}

if (ttype->is_set()) {
t_type* tkey = get_true_type(((t_list*)ttype)->get_elem_type());
t_type* tkey = get_true_type(((t_set*)ttype)->get_elem_type());
if (tkey->is_base_type()) {
t_base_type::t_base tbase = ((t_base_type*)tkey)->get_base();
switch (tbase) {
Expand Down
4 changes: 2 additions & 2 deletions compiler/cpp/src/thrift/generate/t_java_generator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2685,7 +2685,7 @@ void t_java_generator::generate_field_value_meta_data(std::ofstream& out, t_type
} else if (type->is_set()) {
indent(out)
<< "new org.apache.thrift.meta_data.SetMetaData(org.apache.thrift.protocol.TType.SET, ";
t_type* elem_type = ((t_list*)type)->get_elem_type();
t_type* elem_type = ((t_set*)type)->get_elem_type();
generate_field_value_meta_data(out, elem_type);
} else { // map
indent(out)
Expand Down Expand Up @@ -3748,7 +3748,7 @@ void t_java_generator::generate_deserialize_container(ofstream& out,
} else if (ttype->is_list()) {
indent(out) << "org.apache.thrift.protocol.TList " << obj
<< " = new org.apache.thrift.protocol.TList("
<< type_to_enum(((t_set*)ttype)->get_elem_type()) << ", iprot.readI32());"
<< type_to_enum(((t_list*)ttype)->get_elem_type()) << ", iprot.readI32());"
<< endl;
}
}
Expand Down
6 changes: 5 additions & 1 deletion compiler/cpp/src/thrift/generate/t_json_generator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,10 +273,14 @@ void t_json_generator::write_type_spec(t_type* ttype) {
write_key_and_string("valueTypeId", get_type_name(vtype));
write_type_spec_object("keyType", ktype);
write_type_spec_object("valueType", vtype);
} else if (ttype->is_list() || ttype->is_set()) {
} else if (ttype->is_list()) {
t_type* etype = ((t_list*)ttype)->get_elem_type();
write_key_and_string("elemTypeId", get_type_name(etype));
write_type_spec_object("elemType", etype);
} else if (ttype->is_set()) {
t_type* etype = ((t_set*)ttype)->get_elem_type();
write_key_and_string("elemTypeId", get_type_name(etype));
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: these lines are duplicated from above. They can be refactored by setting etype in a conditional and putting 282 and 283 below that.

write_type_spec_object("elemType", etype);
}
}

Expand Down
7 changes: 6 additions & 1 deletion compiler/cpp/src/thrift/generate/t_xml_generator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,11 +391,16 @@ void t_xml_generator::write_type(t_type* ttype) {
if (type == "id") {
write_attribute("type-module", ttype->get_program()->get_name());
write_attribute("type-id", ttype->get_name());
} else if (type == "list" || type == "set") {
} else if (type == "list") {
t_type* etype = ((t_list*)ttype)->get_elem_type();
write_element_start("elemType");
write_type(etype);
write_element_end();
} else if (type == "set") {
t_type* etype = ((t_set*)ttype)->get_elem_type();
write_element_start("elemType");
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: duplicate lines (see above)

Copy link
Member Author

@Jens-G Jens-G Mar 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I know they are duplicates because I duplicated them. One could argue that it is more readable but sure, we can have a conditional. Or we could refactor it into a method and include the map case.

write_type(etype);
write_element_end();
} else if (type == "map") {
t_type* ktype = ((t_map*)ttype)->get_key_type();
write_element_start("keyType");
Expand Down
9 changes: 8 additions & 1 deletion compiler/cpp/src/thrift/parse/t_scope.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "thrift/parse/t_base_type.h"
#include "thrift/parse/t_map.h"
#include "thrift/parse/t_list.h"
#include "thrift/parse/t_set.h"

namespace plugin_output {
template <typename From, typename To>
Expand Down Expand Up @@ -81,12 +82,18 @@ class t_scope {
resolve_const_value(v_iter->first, ((t_map*)ttype)->get_key_type());
resolve_const_value(v_iter->second, ((t_map*)ttype)->get_val_type());
}
} else if (ttype->is_list() || ttype->is_set()) {
} else if (ttype->is_list()) {
const std::vector<t_const_value*>& val = const_val->get_list();
std::vector<t_const_value*>::const_iterator v_iter;
for (v_iter = val.begin(); v_iter != val.end(); ++v_iter) {
resolve_const_value((*v_iter), ((t_list*)ttype)->get_elem_type());
}
} else if (ttype->is_set()) {
const std::vector<t_const_value*>& val = const_val->get_list();
std::vector<t_const_value*>::const_iterator v_iter;
for (v_iter = val.begin(); v_iter != val.end(); ++v_iter) {
resolve_const_value((*v_iter), ((t_set*)ttype)->get_elem_type());
}
} else if (ttype->is_struct()) {
t_struct* tstruct = (t_struct*)ttype;
const std::map<t_const_value*, t_const_value*>& map = const_val->get_map();
Expand Down