Message ID | 20240418205821.366065-1-cupertino.miranda@oracle.com |
---|---|
State | New |
Headers | show |
Series | [v3] bpf: remove huge memory waste with string allocation. | expand |
Hi Cupertino, On 4/18/24 13:58, Cupertino Miranda wrote: > Hi David, everyone, > > Following Davids last review I decided to properly detect error cases, > as suggested. > The error however should be reported earlier in compilation in > pack_enum_valud function, where all the errors are reported. > > Thanks for the quick and detailed reviews. > > Regards, > Cupertino Thanks for taking the time on this. This version is nice, just one little comment: > > The BPF backend was allocating an unnecessarily large string when > constructing CO-RE relocations for enum types. > This patch further verifies if an enumerator is valid for CO-RE > representability and returns an error in those cases. The second sentence is a little awkward and seems to imply the error is returned when the enumerator is valid :) Perhaps "...verifies that an enumerator is valid for CO-RE, and returns an error if it is not" or similar would be more clear? Otherwise, OK. Thanks! > > gcc/ChangeLog: > * config/bpf/core-builtins.cc (get_index_for_enum_value): Create > function. > (pack_enum_value): Check for enumerator and error out. > (process_enum_value): Correct string allocation. > --- > gcc/config/bpf/core-builtins.cc | 57 ++++++++++++++++++++++----------- > 1 file changed, 38 insertions(+), 19 deletions(-) > > diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc > index e03e986e2c1..829acea98f7 100644 > --- a/gcc/config/bpf/core-builtins.cc > +++ b/gcc/config/bpf/core-builtins.cc > @@ -795,6 +795,23 @@ process_field_expr (struct cr_builtins *data) > static GTY(()) hash_map<tree, tree> *bpf_enum_mappings; > tree enum_value_type = NULL_TREE; > > +static int > +get_index_for_enum_value (tree type, tree expr) > +{ > + gcc_assert (TREE_CODE (expr) == CONST_DECL > + && TREE_CODE (type) == ENUMERAL_TYPE); > + > + unsigned int index = 0; > + for (tree l = TYPE_VALUES (type); l; l = TREE_CHAIN (l)) > + { > + gcc_assert (index < (1 << 16)); > + if (TREE_VALUE (l) == expr) > + return index; > + index++; > + } > + return -1; > +} > + > /* Pack helper for the __builtin_preserve_enum_value. */ > > static struct cr_local > @@ -846,6 +863,16 @@ pack_enum_value_fail: > ret.reloc_data.default_value = integer_one_node; > } > > + if (ret.fail == false ) > + { > + int index = get_index_for_enum_value (type, tmp); > + if (index == -1 || index >= (1 << 16)) > + { > + bpf_error ("enum value in CO-RE builtin cannot be represented"); > + ret.fail = true; > + } > + } > + > ret.reloc_data.type = type; > ret.reloc_data.kind = kind; > return ret; > @@ -864,25 +891,17 @@ process_enum_value (struct cr_builtins *data) > > struct cr_final ret = { NULL, type, data->kind }; > > - if (TREE_CODE (expr) == CONST_DECL > - && TREE_CODE (type) == ENUMERAL_TYPE) > - { > - unsigned int index = 0; > - for (tree l = TYPE_VALUES (type); l; l = TREE_CHAIN (l)) > - { > - if (TREE_VALUE (l) == expr) > - { > - char *tmp = (char *) ggc_alloc_atomic ((index / 10) + 1); > - sprintf (tmp, "%d", index); > - ret.str = (const char *) tmp; > - > - break; > - } > - index++; > - } > - } > - else > - gcc_unreachable (); > + gcc_assert (TREE_CODE (expr) == CONST_DECL > + && TREE_CODE (type) == ENUMERAL_TYPE); > + > + int index = get_index_for_enum_value (type, expr); > + gcc_assert (index != -1 && index < (1 << 16)); > + > + /* Index can only be a value up to 2^16. Should always fit > + in 6 chars. */ > + char tmp[6]; > + sprintf (tmp, "%u", index); > + ret.str = CONST_CAST (char *, ggc_strdup(tmp)); > > return ret; > }
David Faust writes: > Hi Cupertino, > > On 4/18/24 13:58, Cupertino Miranda wrote: >> Hi David, everyone, >> >> Following Davids last review I decided to properly detect error cases, >> as suggested. >> The error however should be reported earlier in compilation in >> pack_enum_valud function, where all the errors are reported. >> >> Thanks for the quick and detailed reviews. >> >> Regards, >> Cupertino > > Thanks for taking the time on this. > This version is nice, just one little comment: > >> >> The BPF backend was allocating an unnecessarily large string when >> constructing CO-RE relocations for enum types. >> This patch further verifies if an enumerator is valid for CO-RE >> representability and returns an error in those cases. > > The second sentence is a little awkward and seems to imply the error is > returned when the enumerator is valid :) > Perhaps "...verifies that an enumerator is valid for CO-RE, and returns > an error if it is not" or similar would be more clear? Thanks for all the suggestions. > > Otherwise, OK. > Thanks! Pushed! > > >> >> gcc/ChangeLog: >> * config/bpf/core-builtins.cc (get_index_for_enum_value): Create >> function. >> (pack_enum_value): Check for enumerator and error out. >> (process_enum_value): Correct string allocation. >> --- >> gcc/config/bpf/core-builtins.cc | 57 ++++++++++++++++++++++----------- >> 1 file changed, 38 insertions(+), 19 deletions(-) >> >> diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc >> index e03e986e2c1..829acea98f7 100644 >> --- a/gcc/config/bpf/core-builtins.cc >> +++ b/gcc/config/bpf/core-builtins.cc >> @@ -795,6 +795,23 @@ process_field_expr (struct cr_builtins *data) >> static GTY(()) hash_map<tree, tree> *bpf_enum_mappings; >> tree enum_value_type = NULL_TREE; >> >> +static int >> +get_index_for_enum_value (tree type, tree expr) >> +{ >> + gcc_assert (TREE_CODE (expr) == CONST_DECL >> + && TREE_CODE (type) == ENUMERAL_TYPE); >> + >> + unsigned int index = 0; >> + for (tree l = TYPE_VALUES (type); l; l = TREE_CHAIN (l)) >> + { >> + gcc_assert (index < (1 << 16)); >> + if (TREE_VALUE (l) == expr) >> + return index; >> + index++; >> + } >> + return -1; >> +} >> + >> /* Pack helper for the __builtin_preserve_enum_value. */ >> >> static struct cr_local >> @@ -846,6 +863,16 @@ pack_enum_value_fail: >> ret.reloc_data.default_value = integer_one_node; >> } >> >> + if (ret.fail == false ) >> + { >> + int index = get_index_for_enum_value (type, tmp); >> + if (index == -1 || index >= (1 << 16)) >> + { >> + bpf_error ("enum value in CO-RE builtin cannot be represented"); >> + ret.fail = true; >> + } >> + } >> + >> ret.reloc_data.type = type; >> ret.reloc_data.kind = kind; >> return ret; >> @@ -864,25 +891,17 @@ process_enum_value (struct cr_builtins *data) >> >> struct cr_final ret = { NULL, type, data->kind }; >> >> - if (TREE_CODE (expr) == CONST_DECL >> - && TREE_CODE (type) == ENUMERAL_TYPE) >> - { >> - unsigned int index = 0; >> - for (tree l = TYPE_VALUES (type); l; l = TREE_CHAIN (l)) >> - { >> - if (TREE_VALUE (l) == expr) >> - { >> - char *tmp = (char *) ggc_alloc_atomic ((index / 10) + 1); >> - sprintf (tmp, "%d", index); >> - ret.str = (const char *) tmp; >> - >> - break; >> - } >> - index++; >> - } >> - } >> - else >> - gcc_unreachable (); >> + gcc_assert (TREE_CODE (expr) == CONST_DECL >> + && TREE_CODE (type) == ENUMERAL_TYPE); >> + >> + int index = get_index_for_enum_value (type, expr); >> + gcc_assert (index != -1 && index < (1 << 16)); >> + >> + /* Index can only be a value up to 2^16. Should always fit >> + in 6 chars. */ >> + char tmp[6]; >> + sprintf (tmp, "%u", index); >> + ret.str = CONST_CAST (char *, ggc_strdup(tmp)); >> >> return ret; >> }
diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc index e03e986e2c1..829acea98f7 100644 --- a/gcc/config/bpf/core-builtins.cc +++ b/gcc/config/bpf/core-builtins.cc @@ -795,6 +795,23 @@ process_field_expr (struct cr_builtins *data) static GTY(()) hash_map<tree, tree> *bpf_enum_mappings; tree enum_value_type = NULL_TREE; +static int +get_index_for_enum_value (tree type, tree expr) +{ + gcc_assert (TREE_CODE (expr) == CONST_DECL + && TREE_CODE (type) == ENUMERAL_TYPE); + + unsigned int index = 0; + for (tree l = TYPE_VALUES (type); l; l = TREE_CHAIN (l)) + { + gcc_assert (index < (1 << 16)); + if (TREE_VALUE (l) == expr) + return index; + index++; + } + return -1; +} + /* Pack helper for the __builtin_preserve_enum_value. */ static struct cr_local @@ -846,6 +863,16 @@ pack_enum_value_fail: ret.reloc_data.default_value = integer_one_node; } + if (ret.fail == false ) + { + int index = get_index_for_enum_value (type, tmp); + if (index == -1 || index >= (1 << 16)) + { + bpf_error ("enum value in CO-RE builtin cannot be represented"); + ret.fail = true; + } + } + ret.reloc_data.type = type; ret.reloc_data.kind = kind; return ret; @@ -864,25 +891,17 @@ process_enum_value (struct cr_builtins *data) struct cr_final ret = { NULL, type, data->kind }; - if (TREE_CODE (expr) == CONST_DECL - && TREE_CODE (type) == ENUMERAL_TYPE) - { - unsigned int index = 0; - for (tree l = TYPE_VALUES (type); l; l = TREE_CHAIN (l)) - { - if (TREE_VALUE (l) == expr) - { - char *tmp = (char *) ggc_alloc_atomic ((index / 10) + 1); - sprintf (tmp, "%d", index); - ret.str = (const char *) tmp; - - break; - } - index++; - } - } - else - gcc_unreachable (); + gcc_assert (TREE_CODE (expr) == CONST_DECL + && TREE_CODE (type) == ENUMERAL_TYPE); + + int index = get_index_for_enum_value (type, expr); + gcc_assert (index != -1 && index < (1 << 16)); + + /* Index can only be a value up to 2^16. Should always fit + in 6 chars. */ + char tmp[6]; + sprintf (tmp, "%u", index); + ret.str = CONST_CAST (char *, ggc_strdup(tmp)); return ret; }