| Message ID | 1448974501-30981-2-git-send-email-rep.dot.nop@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Dec 1, 2015 at 2:54 PM, Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > These three function used a hardcoded buffer of 100 but would be better > off to base off GFC_MAX_SYMBOL_LEN which denotes the maximum length of a > name in any of our supported standards (63 as of f2003 ff.). Please use xasprintf() instead (and free the result, or course). One of my backburner projects is to get rid of these static symbol buffers, and use dynamic buffers (or the symbol table) instead. We IIRC already have some ugly hacks by using hashing to get around GFC_MAX_SYMBOL_LEN when handling mangled symbols. Your patch doesn't make the situation worse per se, but if you're going to fix it, lets do it properly. Ok for GCC 7 stage1 with these changes. I don't think it's worth putting it into GCC 6 at this point anymore, unless this is actually fixing some bugs that are visible to users?
On 1 December 2015 at 15:52, Janne Blomqvist <blomqvist.janne@gmail.com> wrote: > On Tue, Dec 1, 2015 at 2:54 PM, Bernhard Reutner-Fischer > <rep.dot.nop@gmail.com> wrote: >> These three function used a hardcoded buffer of 100 but would be better >> off to base off GFC_MAX_SYMBOL_LEN which denotes the maximum length of a >> name in any of our supported standards (63 as of f2003 ff.). > > Please use xasprintf() instead (and free the result, or course). One > of my backburner projects is to get rid of these static symbol > buffers, and use dynamic buffers (or the symbol table) instead. We > IIRC already have some ugly hacks by using hashing to get around > GFC_MAX_SYMBOL_LEN when handling mangled symbols. Your patch doesn't > make the situation worse per se, but if you're going to fix it, lets > do it properly. I see. /scratch/src/gcc-6.0.mine/gcc/fortran$ git grep "^[[:space:]]*char[[:space:]][[:space:]]*[^[;[:space:]]*\[" | wc -l 142 /scratch/src/gcc-6.0.mine/gcc/fortran$ git grep "xasprintf" | wc -l 32 What about memory fragmentation when switching to heap-based allocation? Or is there consensus that these are in the noise compared to other parts of the compiler? BTW: $ git grep APO io.c: static const char *delim[] = { "APOSTROPHE", "QUOTE", "NONE", NULL }; io.c: static const char *delim[] = { "APOSTROPHE", "QUOTE", "NONE", NULL }; > Ok for GCC 7 stage1 with these changes. I don't think it's worth > putting it into GCC 6 at this point anymore, unless this is actually > fixing some bugs that are visible to users? Not visible, no, can wait easily.
On Tue, Dec 1, 2015 at 6:51 PM, Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > On 1 December 2015 at 15:52, Janne Blomqvist <blomqvist.janne@gmail.com> wrote: >> On Tue, Dec 1, 2015 at 2:54 PM, Bernhard Reutner-Fischer >> <rep.dot.nop@gmail.com> wrote: >>> These three function used a hardcoded buffer of 100 but would be better >>> off to base off GFC_MAX_SYMBOL_LEN which denotes the maximum length of a >>> name in any of our supported standards (63 as of f2003 ff.). >> >> Please use xasprintf() instead (and free the result, or course). One >> of my backburner projects is to get rid of these static symbol >> buffers, and use dynamic buffers (or the symbol table) instead. We >> IIRC already have some ugly hacks by using hashing to get around >> GFC_MAX_SYMBOL_LEN when handling mangled symbols. Your patch doesn't >> make the situation worse per se, but if you're going to fix it, lets >> do it properly. > > I see. > > /scratch/src/gcc-6.0.mine/gcc/fortran$ git grep > "^[[:space:]]*char[[:space:]][[:space:]]*[^[;[:space:]]*\[" | wc -l > 142 > /scratch/src/gcc-6.0.mine/gcc/fortran$ git grep "xasprintf" | wc -l > 32 Yes, that's why it's on the TODO-list rather than on the DONE-list. :) > What about memory fragmentation when switching to heap-based allocation? > Or is there consensus that these are in the noise compared to other > parts of the compiler? Heap fragmentation is an issue, yes. I'm not sure it's that performance-critical, but I don't think there is any consensus. I just want to avoid ugly hacks like symbol hashing to fit within some fixed buffer. Perhaps an good compromise would be something like std::string with small string optimization, but as you have seen there is some resistance to C++. But this is more relevant for mangled symbols, so GFC_MAX_MANGLED_SYMBOL_LEN is more relevant here, and there's only a few of them left. So, well, if you're sure that mangled symbols are never copied into the buffers your patch modifies, please consider your original patch Ok as well. Whichever you prefer. Performance-wise I think a bigger benefit would be to use the symbol table more and then e.g. be able to do pointer comparisons rather than strcmp(). But that is certainly much more work. > BTW: > $ git grep APO > io.c: static const char *delim[] = { "APOSTROPHE", "QUOTE", "NONE", NULL }; > io.c: static const char *delim[] = { "APOSTROPHE", "QUOTE", "NONE", NULL }; ? What are you saying?
On December 3, 2015 10:46:09 AM GMT+01:00, Janne Blomqvist <blomqvist.janne@gmail.com> wrote: >On Tue, Dec 1, 2015 at 6:51 PM, Bernhard Reutner-Fischer ><rep.dot.nop@gmail.com> wrote: >> On 1 December 2015 at 15:52, Janne Blomqvist ><blomqvist.janne@gmail.com> wrote: >>> On Tue, Dec 1, 2015 at 2:54 PM, Bernhard Reutner-Fischer >>> <rep.dot.nop@gmail.com> wrote: >>>> These three function used a hardcoded buffer of 100 but would be >better >>>> off to base off GFC_MAX_SYMBOL_LEN which denotes the maximum length >of a >>>> name in any of our supported standards (63 as of f2003 ff.). >>> >>> Please use xasprintf() instead (and free the result, or course). One >>> of my backburner projects is to get rid of these static symbol >>> buffers, and use dynamic buffers (or the symbol table) instead. We >>> IIRC already have some ugly hacks by using hashing to get around >>> GFC_MAX_SYMBOL_LEN when handling mangled symbols. Your patch doesn't >>> make the situation worse per se, but if you're going to fix it, lets >>> do it properly. >> >> I see. >> >> /scratch/src/gcc-6.0.mine/gcc/fortran$ git grep >> "^[[:space:]]*char[[:space:]][[:space:]]*[^[;[:space:]]*\[" | wc -l >> 142 >> /scratch/src/gcc-6.0.mine/gcc/fortran$ git grep "xasprintf" | wc -l >> 32 > >Yes, that's why it's on the TODO-list rather than on the DONE-list. :) > >> What about memory fragmentation when switching to heap-based >allocation? >> Or is there consensus that these are in the noise compared to other >> parts of the compiler? > >Heap fragmentation is an issue, yes. I'm not sure it's that >performance-critical, but I don't think there is any consensus. I just >want to avoid ugly hacks like symbol hashing to fit within some fixed >buffer. Perhaps an good compromise would be something like std::string >with small string optimization, but as you have seen there is some >resistance to C++. But this is more relevant for mangled symbols, so >GFC_MAX_MANGLED_SYMBOL_LEN is more relevant here, and there's only a >few of them left. So, well, if you're sure that mangled symbols are >never copied into the buffers your patch modifies, please consider >your original patch Ok as well. Whichever you prefer. > >Performance-wise I think a bigger benefit would be to use the symbol >table more and then e.g. be able to do pointer comparisons rather than >strcmp(). But that is certainly much more work. Hm, worth a look indeed since that would certainly be a step in the right direction. > >> BTW: >> $ git grep APO >> io.c: static const char *delim[] = { "APOSTROPHE", "QUOTE", "NONE", >NULL }; >> io.c: static const char *delim[] = { "APOSTROPHE", "QUOTE", "NONE", >NULL }; > >? What are you saying? delim is duplicated, we should remove one instance. thanks,
On Sat, Jun 18, 2016 at 09:46:17PM +0200, Bernhard Reutner-Fischer wrote: > On December 3, 2015 10:46:09 AM GMT+01:00, Janne Blomqvist <blomqvist.janne@gmail.com> wrote: > >On Tue, Dec 1, 2015 at 6:51 PM, Bernhard Reutner-Fischer > ><rep.dot.nop@gmail.com> wrote: > >> On 1 December 2015 at 15:52, Janne Blomqvist > ><blomqvist.janne@gmail.com> wrote: > >>> On Tue, Dec 1, 2015 at 2:54 PM, Bernhard Reutner-Fischer > >>> <rep.dot.nop@gmail.com> wrote: > >>>> These three function used a hardcoded buffer of 100 but would be > >better > >>>> off to base off GFC_MAX_SYMBOL_LEN which denotes the maximum length > >of a > >>>> name in any of our supported standards (63 as of f2003 ff.). > >>> > >>> Please use xasprintf() instead (and free the result, or course). One > >>> of my backburner projects is to get rid of these static symbol > >>> buffers, and use dynamic buffers (or the symbol table) instead. We > >>> IIRC already have some ugly hacks by using hashing to get around > >>> GFC_MAX_SYMBOL_LEN when handling mangled symbols. Your patch doesn't > >>> make the situation worse per se, but if you're going to fix it, lets > >>> do it properly. > >> > >> I see. > >> > >> /scratch/src/gcc-6.0.mine/gcc/fortran$ git grep > >> "^[[:space:]]*char[[:space:]][[:space:]]*[^[;[:space:]]*\[" | wc -l > >> 142 > >> /scratch/src/gcc-6.0.mine/gcc/fortran$ git grep "xasprintf" | wc -l > >> 32 > > > >Yes, that's why it's on the TODO-list rather than on the DONE-list. :) > > > >> What about memory fragmentation when switching to heap-based > >allocation? > >> Or is there consensus that these are in the noise compared to other > >> parts of the compiler? > > > >Heap fragmentation is an issue, yes. I'm not sure it's that > >performance-critical, but I don't think there is any consensus. I just > >want to avoid ugly hacks like symbol hashing to fit within some fixed > >buffer. Perhaps an good compromise would be something like std::string > >with small string optimization, but as you have seen there is some > >resistance to C++. But this is more relevant for mangled symbols, so > >GFC_MAX_MANGLED_SYMBOL_LEN is more relevant here, and there's only a > >few of them left. So, well, if you're sure that mangled symbols are > >never copied into the buffers your patch modifies, please consider > >your original patch Ok as well. Whichever you prefer. > > > >Performance-wise I think a bigger benefit would be to use the symbol > >table more and then e.g. be able to do pointer comparisons rather than > >strcmp(). But that is certainly much more work. > > Hm, worth a look indeed since that would certainly be a step in the right direction. Installed the initial patch as intermediate step as r253881 for now. thanks, > > > > >> BTW: > >> $ git grep APO > >> io.c: static const char *delim[] = { "APOSTROPHE", "QUOTE", "NONE", > >NULL }; > >> io.c: static const char *delim[] = { "APOSTROPHE", "QUOTE", "NONE", > >NULL }; > > > >? What are you saying? > > delim is duplicated, we should remove one instance. > thanks, >
On 19 October 2017 10:03:06 CEST, Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: >On Sat, Jun 18, 2016 at 09:46:17PM +0200, Bernhard Reutner-Fischer >wrote: >> On December 3, 2015 10:46:09 AM GMT+01:00, Janne Blomqvist ><blomqvist.janne@gmail.com> wrote: >> >On Tue, Dec 1, 2015 at 6:51 PM, Bernhard Reutner-Fischer >> ><rep.dot.nop@gmail.com> wrote: >> >> On 1 December 2015 at 15:52, Janne Blomqvist >> ><blomqvist.janne@gmail.com> wrote: >> >>> On Tue, Dec 1, 2015 at 2:54 PM, Bernhard Reutner-Fischer >> >>> <rep.dot.nop@gmail.com> wrote: >> >>>> These three function used a hardcoded buffer of 100 but would be >> >better >> >>>> off to base off GFC_MAX_SYMBOL_LEN which denotes the maximum >length >> >of a >> >>>> name in any of our supported standards (63 as of f2003 ff.). >> >>> >> >>> Please use xasprintf() instead (and free the result, or course). >One >> >>> of my backburner projects is to get rid of these static symbol >> >>> buffers, and use dynamic buffers (or the symbol table) instead. >We >> >>> IIRC already have some ugly hacks by using hashing to get around >> >>> GFC_MAX_SYMBOL_LEN when handling mangled symbols. Your patch >doesn't >> >>> make the situation worse per se, but if you're going to fix it, >lets >> >>> do it properly. >> >> >> >> I see. >> >> >> >> /scratch/src/gcc-6.0.mine/gcc/fortran$ git grep >> >> "^[[:space:]]*char[[:space:]][[:space:]]*[^[;[:space:]]*\[" | wc >-l >> >> 142 >> >> /scratch/src/gcc-6.0.mine/gcc/fortran$ git grep "xasprintf" | wc >-l >> >> 32 >> > >> >Yes, that's why it's on the TODO-list rather than on the DONE-list. >:) >> > >> >> What about memory fragmentation when switching to heap-based >> >allocation? >> >> Or is there consensus that these are in the noise compared to >other >> >> parts of the compiler? >> > >> >Heap fragmentation is an issue, yes. I'm not sure it's that >> >performance-critical, but I don't think there is any consensus. I >just >> >want to avoid ugly hacks like symbol hashing to fit within some >fixed >> >buffer. Perhaps an good compromise would be something like >std::string >> >with small string optimization, but as you have seen there is some >> >resistance to C++. But this is more relevant for mangled symbols, so >> >GFC_MAX_MANGLED_SYMBOL_LEN is more relevant here, and there's only a >> >few of them left. So, well, if you're sure that mangled symbols are >> >never copied into the buffers your patch modifies, please consider >> >your original patch Ok as well. Whichever you prefer. >> > >> >Performance-wise I think a bigger benefit would be to use the symbol >> >table more and then e.g. be able to do pointer comparisons rather >than >> >strcmp(). But that is certainly much more work. >> >> Hm, worth a look indeed since that would certainly be a step in the >right direction. > >Installed the initial patch as intermediate step as r253881 for now. JFYI I'm contemplating to move the stack-based allocations to heap-based ones now, starting with gfc_match_name and gradually moving to pointer comparisons with the stringpool based identifiers. I'll strive to suggest something for discussion in smallish steps when it's ready. Cheers, > >thanks, >> >> > >> >> BTW: >> >> $ git grep APO >> >> io.c: static const char *delim[] = { "APOSTROPHE", "QUOTE", >"NONE", >> >NULL }; >> >> io.c: static const char *delim[] = { "APOSTROPHE", "QUOTE", >"NONE", >> >NULL }; >> > >> >? What are you saying? >> >> delim is duplicated, we should remove one instance. >> thanks, >>
diff --git a/gcc/fortran/interface.c b/gcc/fortran/interface.c index dcf3eae..30cc522 100644 --- a/gcc/fortran/interface.c +++ b/gcc/fortran/interface.c @@ -1696,7 +1696,7 @@ check_interface1 (gfc_interface *p, gfc_interface *q0, static void check_sym_interfaces (gfc_symbol *sym) { - char interface_name[100]; + char interface_name[GFC_MAX_SYMBOL_LEN + sizeof("generic interface ''")]; gfc_interface *p; if (sym->ns != gfc_current_ns) @@ -1733,7 +1733,7 @@ check_sym_interfaces (gfc_symbol *sym) static void check_uop_interfaces (gfc_user_op *uop) { - char interface_name[100]; + char interface_name[GFC_MAX_SYMBOL_LEN + sizeof("operator interface ''")]; gfc_user_op *uop2; gfc_namespace *ns; @@ -1810,7 +1810,7 @@ void gfc_check_interfaces (gfc_namespace *ns) { gfc_namespace *old_ns, *ns2; - char interface_name[100]; + char interface_name[GFC_MAX_SYMBOL_LEN + sizeof("intrinsic '' operator")]; int i; old_ns = gfc_current_ns;
These three function used a hardcoded buffer of 100 but would be better off to base off GFC_MAX_SYMBOL_LEN which denotes the maximum length of a name in any of our supported standards (63 as of f2003 ff.). Regstrapped without regressions, ok for trunk stage3 now / next stage1? gcc/fortran/ChangeLog 2015-11-29 Bernhard Reutner-Fischer <aldot@gcc.gnu.org> * interface.c (check_sym_interfaces, check_uop_interfaces, gfc_check_interfaces): Base interface_name buffer off GFC_MAX_SYMBOL_LEN. Signed-off-by: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> --- gcc/fortran/interface.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)