| Message ID | 20240411111118.215612-2-cupertino.miranda@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] bpf: support more instructions to match CO-RE relocations | expand |
Hi Cupertino, On 4/11/24 04:11, Cupertino Miranda wrote: > Code was allocating way too much space for the string. A little bit more description would not hurt. Perhaps you could say something like: "The BPF backend was allocating an unnecessarily large string when constructing CO-RE relocations for enum types." > > gcc/ChangeLog: > * config/bpf/core-builtins.cc (process_enum_value): Corrected > string allocation. nit: present tense, i.e. "Correct" rather than "Corrected" > --- > gcc/config/bpf/core-builtins.cc | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc > index e03e986e2c1..ead1777d465 100644 > --- a/gcc/config/bpf/core-builtins.cc > +++ b/gcc/config/bpf/core-builtins.cc > @@ -872,10 +872,11 @@ process_enum_value (struct cr_builtins *data) > { > if (TREE_VALUE (l) == expr) > { > - char *tmp = (char *) ggc_alloc_atomic ((index / 10) + 1); > + /* Array size is 21 = ceil(log_10(2^64)) + 1 to hold string > + representations of 64 bit integers. */ > + char tmp[21]; > sprintf (tmp, "%d", index); It looks like `index' is an `unsigned int', so this sprintf should use %u rather %d, no? Also, it occurs to me that the `vlen' of BTF types is only 16 bits, so BTF has no way currently to represent enums with more than 65535 enumerators. It may be worth adding a sanity check to bail out (error) here if we're going to claim an index higher than that. And if that is validated before the printf, the buffer can be 6 bytes ("65535\0"). > - ret.str = (const char *) tmp; > - > + ret.str = CONST_CAST (char *, ggc_strdup(tmp)); > break; > } > index++;
Hi David, Thanks for the review. Will apply the changes. Nice catch and optimization with the vlen size. Cupertino David Faust writes: > Hi Cupertino, > > On 4/11/24 04:11, Cupertino Miranda wrote: >> Code was allocating way too much space for the string. > > A little bit more description would not hurt. Perhaps you could say > something like: > "The BPF backend was allocating an unnecessarily large string when > constructing CO-RE relocations for enum types." > >> >> gcc/ChangeLog: >> * config/bpf/core-builtins.cc (process_enum_value): Corrected >> string allocation. > > nit: present tense, i.e. "Correct" rather than "Corrected" > >> --- >> gcc/config/bpf/core-builtins.cc | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc >> index e03e986e2c1..ead1777d465 100644 >> --- a/gcc/config/bpf/core-builtins.cc >> +++ b/gcc/config/bpf/core-builtins.cc >> @@ -872,10 +872,11 @@ process_enum_value (struct cr_builtins *data) >> { >> if (TREE_VALUE (l) == expr) >> { >> - char *tmp = (char *) ggc_alloc_atomic ((index / 10) + 1); >> + /* Array size is 21 = ceil(log_10(2^64)) + 1 to hold string >> + representations of 64 bit integers. */ >> + char tmp[21]; >> sprintf (tmp, "%d", index); > > It looks like `index' is an `unsigned int', so this sprintf should use > %u rather %d, no? > > Also, it occurs to me that the `vlen' of BTF types is only 16 bits, so > BTF has no way currently to represent enums with more than 65535 > enumerators. It may be worth adding a sanity check to bail out (error) > here if we're going to claim an index higher than that. And if that is > validated before the printf, the buffer can be 6 bytes ("65535\0"). > >> - ret.str = (const char *) tmp; >> - >> + ret.str = CONST_CAST (char *, ggc_strdup(tmp)); >> break; >> } >> index++;
diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc index e03e986e2c1..ead1777d465 100644 --- a/gcc/config/bpf/core-builtins.cc +++ b/gcc/config/bpf/core-builtins.cc @@ -872,10 +872,11 @@ process_enum_value (struct cr_builtins *data) { if (TREE_VALUE (l) == expr) { - char *tmp = (char *) ggc_alloc_atomic ((index / 10) + 1); + /* Array size is 21 = ceil(log_10(2^64)) + 1 to hold string + representations of 64 bit integers. */ + char tmp[21]; sprintf (tmp, "%d", index); - ret.str = (const char *) tmp; - + ret.str = CONST_CAST (char *, ggc_strdup(tmp)); break; } index++;