| Message ID | 20211105211718.2261686-3-rep.dot.nop@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | incpath, Fortran: Fix memory leak in gfc_add_include_path | expand |
Hi Bernhard, I cannot comment on the gcc/ parts, but Am 05.11.21 um 22:17 schrieb Bernhard Reutner-Fischer via Gcc-patches: > From: Bernhard Reutner-Fischer <aldot@gcc.gnu.org> > > gcc/fortran/ChangeLog: > > PR fortran/68800 > * cpp.h (gfc_cpp_free_cpp_dirs): New declaration. > * cpp.c (gfc_cpp_free_cpp_dirs): New definition. > (gfc_cpp_add_include_path, gfc_cpp_add_include_path_after): Add > comment. > * f95-lang.c (gfc_finish): Call gfc_cpp_free_cpp_dirs. > * scanner.c (add_path_to_list): Allocate unzeroed memory. > (gfc_add_include_path): Add comment and fix formatting. > > --- > Bootstrapped and regtested without regressions on x86_64-unknown-linux. > Ok for trunk? > > Note: in add_path_to_list i changed dir->path = XCNEWVEC to XNEWVEC > since strcpy and strcat add a terminating null byte for us anyway. > Fixes: > > -== 68 bytes in 1 blocks are still reachable in loss record 228 of 371 > -== at : malloc (vg_replace_malloc.c:380) > -== by : xmalloc (xmalloc.c:149) > -== by : xstrdup (xstrdup.c:34) > -== by : gfc_add_include_path(char const*, bool, bool, bool, bool) (scanner.c > :428) > -== by : gfc_handle_option(unsigned long, char const*, long, int, unsigned in > t, cl_option_handlers const*) (options.c:702) > -== by : handle_option(gcc_options*, gcc_options*, cl_decoded_option const*, > unsigned int, int, unsigned int, cl_option_handlers const*, bool, diagnostic_con > text*) (opts-common.c:1181) > -== by : read_cmdline_option(gcc_options*, gcc_options*, cl_decoded_option*, > unsigned int, unsigned int, cl_option_handlers const*, diagnostic_context*) (opt > s-common.c:1431) > -== by : read_cmdline_options (opts-global.c:237) > -== by : decode_options(gcc_options*, gcc_options*, cl_decoded_option*, unsig > ned int, unsigned int, diagnostic_context*, void (*)()) (opts-global.c:319) > -== by : toplev::main(int, char**) (toplev.c:2273) > -== by : main (main.c:39) > --- > gcc/fortran/cpp.c | 13 +++++++++++-- > gcc/fortran/cpp.h | 1 + > gcc/fortran/f95-lang.c | 2 +- > gcc/fortran/scanner.c | 7 ++++--- > 4 files changed, 17 insertions(+), 6 deletions(-) > > diff --git a/gcc/fortran/cpp.c b/gcc/fortran/cpp.c > index e86386c8b17..04fe8fe460b 100644 > --- a/gcc/fortran/cpp.c > +++ b/gcc/fortran/cpp.c > @@ -728,12 +728,20 @@ gfc_cpp_done (void) > cpp_clear_file_cache (cpp_in); > } why do you introduce a wrapper for something outside of fortran that is used only once, > -/* PATH must be malloc-ed and NULL-terminated. */ > +/* Free all cpp include dirs. */ > +void > +gfc_cpp_free_cpp_dirs (void) > +{ > + free_cpp_dirs (); > +} > + > +/* PATH must be NULL-terminated. */ > void > gfc_cpp_add_include_path (char *path, bool user_supplied) > { > /* CHAIN sets cpp_dir->sysp which differs from 0 if PATH is a system > - include path. Fortran does not define any system include paths. */ > + include path. Fortran does not define any system include paths. > + incpath.c manages the incoming, xstrdup()ed path. */ > int cxx_aware = 0; > > add_path (path, INC_BRACKET, cxx_aware, user_supplied); > @@ -742,6 +750,7 @@ gfc_cpp_add_include_path (char *path, bool user_supplied) > void > gfc_cpp_add_include_path_after (char *path, bool user_supplied) > { > + /* incpath.c manages the incoming, xstrdup()ed path. */ > int cxx_aware = 0; > add_path (path, INC_AFTER, cxx_aware, user_supplied); > } > diff --git a/gcc/fortran/cpp.h b/gcc/fortran/cpp.h > index 44644a2a333..963b9a9c89e 100644 > --- a/gcc/fortran/cpp.h > +++ b/gcc/fortran/cpp.h > @@ -46,6 +46,7 @@ void gfc_cpp_post_options (bool); > bool gfc_cpp_preprocess (const char *source_file); > > void gfc_cpp_done (void); > +void gfc_cpp_free_cpp_dirs (void); > > void gfc_cpp_add_include_path (char *path, bool user_supplied); > void gfc_cpp_add_include_path_after (char *path, bool user_supplied); > diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c > index 58dcaf01d75..ec4c2cf01d9 100644 > --- a/gcc/fortran/f95-lang.c > +++ b/gcc/fortran/f95-lang.c > @@ -275,7 +275,7 @@ gfc_finish (void) > gfc_cpp_done (); > gfc_done_1 (); > gfc_release_include_path (); > - return; namely here? > + gfc_cpp_free_cpp_dirs (); > } Why not call free_cpp_dirs () here directly, omit all unnecessary stuff, and maybe only add a brief comment here? > /* These functions and variables deal with binding contours. We only > diff --git a/gcc/fortran/scanner.c b/gcc/fortran/scanner.c > index 69b81ab97f8..268d1e3e7fb 100644 > --- a/gcc/fortran/scanner.c > +++ b/gcc/fortran/scanner.c > @@ -370,7 +370,7 @@ add_path_to_list (gfc_directorylist **list, const char *path, > char *q; > size_t len; > int i; > - > + > p = path; > while (*p == ' ' || *p == '\t') /* someone might do "-I include" */ > if (*p++ == '\0') > @@ -409,7 +409,7 @@ add_path_to_list (gfc_directorylist **list, const char *path, > *list = dir; > dir->use_for_modules = use_for_modules; > dir->warn = warn; > - dir->path = XCNEWVEC (char, strlen (p) + 2); > + dir->path = XNEWVEC (char, strlen (p) + 2); > strcpy (dir->path, p); > strcat (dir->path, "/"); /* make '/' last character */ > } > @@ -424,8 +424,9 @@ gfc_add_include_path (const char *path, bool use_for_modules, bool file_dir, > defer_warn); > > /* For '#include "..."' these directories are automatically searched. */ > + /* CPP manages 'path' on it's own via incpath.c so give it it's own copy. */ > if (!file_dir) > - gfc_cpp_add_include_path (xstrdup(path), true); > + gfc_cpp_add_include_path (xstrdup (path), true); > } > > > Harald
On Sat, 6 Nov 2021 20:22:53 +0100 Harald Anlauf <anlauf@gmx.de> wrote: > Hi Bernhard, > > I cannot comment on the gcc/ parts, but > > > diff --git a/gcc/fortran/cpp.c b/gcc/fortran/cpp.c > > index e86386c8b17..04fe8fe460b 100644 > > --- a/gcc/fortran/cpp.c > > +++ b/gcc/fortran/cpp.c > > @@ -728,12 +728,20 @@ gfc_cpp_done (void) > > cpp_clear_file_cache (cpp_in); > > } > > why do you introduce a wrapper for something outside of fortran > that is used only once, > > > -/* PATH must be malloc-ed and NULL-terminated. */ > > +/* Free all cpp include dirs. */ > > +void > > +gfc_cpp_free_cpp_dirs (void) > > +{ > > + free_cpp_dirs (); > > +} > > diff --git a/gcc/fortran/cpp.h b/gcc/fortran/cpp.h > > index 44644a2a333..963b9a9c89e 100644 > > --- a/gcc/fortran/cpp.h > > +++ b/gcc/fortran/cpp.h > > @@ -46,6 +46,7 @@ void gfc_cpp_post_options (bool); > > bool gfc_cpp_preprocess (const char *source_file); > > > > void gfc_cpp_done (void); > > +void gfc_cpp_free_cpp_dirs (void); > > > > void gfc_cpp_add_include_path (char *path, bool user_supplied); > > void gfc_cpp_add_include_path_after (char *path, bool user_supplied); > > diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c > > index 58dcaf01d75..ec4c2cf01d9 100644 > > --- a/gcc/fortran/f95-lang.c > > +++ b/gcc/fortran/f95-lang.c > > @@ -275,7 +275,7 @@ gfc_finish (void) > > gfc_cpp_done (); > > gfc_done_1 (); > > gfc_release_include_path (); > > - return; > > namely here? > > > + gfc_cpp_free_cpp_dirs (); > > } > > Why not call free_cpp_dirs () here directly, omit all unnecessary > stuff, and maybe only add a brief comment here? cpp.c includes incpath.h, f95-lang.c does not and should not. So the cleanest thing is to keep the cpp handling in cpp.[ch] and have the language frontend call into it's cpp bits. It would be rather rogue to extern void free_cpp_dirs (void); in f95-lang.c and directly call it in gfc_finish, i'd say? thanks,
ping? libcpp maintainers, is the helper in incpath.* ok? fortraners, Do you prefer a rogue, local forward declaration, or is the introduction of that trivial wrapper ok? I don't think pulling in cpp.h from f95-lang.cc is desirable, but i can do that if you all think that's preferred. cover-letter: https://gcc.gnu.org/pipermail/gcc-patches/2021-November/583522.html gcc/incpath.* bits (i guess that'd be for the libcpp maintainers): https://gcc.gnu.org/pipermail/gcc-patches/2021-November/583520.html fortran bits: https://gcc.gnu.org/pipermail/gcc-patches/2021-November/583521.html thanks, On Sun, 7 Nov 2021 02:38:21 +0100 Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > On Sat, 6 Nov 2021 20:22:53 +0100 > Harald Anlauf <anlauf@gmx.de> wrote: > > > Hi Bernhard, > > > > I cannot comment on the gcc/ parts, but > > > > > > diff --git a/gcc/fortran/cpp.c b/gcc/fortran/cpp.c > > > index e86386c8b17..04fe8fe460b 100644 > > > --- a/gcc/fortran/cpp.c > > > +++ b/gcc/fortran/cpp.c > > > @@ -728,12 +728,20 @@ gfc_cpp_done (void) > > > cpp_clear_file_cache (cpp_in); > > > } > > > > why do you introduce a wrapper for something outside of fortran > > that is used only once, > > > > > -/* PATH must be malloc-ed and NULL-terminated. */ > > > +/* Free all cpp include dirs. */ > > > +void > > > +gfc_cpp_free_cpp_dirs (void) > > > +{ > > > + free_cpp_dirs (); > > > +} > > > > diff --git a/gcc/fortran/cpp.h b/gcc/fortran/cpp.h > > > index 44644a2a333..963b9a9c89e 100644 > > > --- a/gcc/fortran/cpp.h > > > +++ b/gcc/fortran/cpp.h > > > @@ -46,6 +46,7 @@ void gfc_cpp_post_options (bool); > > > bool gfc_cpp_preprocess (const char *source_file); > > > > > > void gfc_cpp_done (void); > > > +void gfc_cpp_free_cpp_dirs (void); > > > > > > void gfc_cpp_add_include_path (char *path, bool user_supplied); > > > void gfc_cpp_add_include_path_after (char *path, bool user_supplied); > > > diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c > > > index 58dcaf01d75..ec4c2cf01d9 100644 > > > --- a/gcc/fortran/f95-lang.c > > > +++ b/gcc/fortran/f95-lang.c > > > @@ -275,7 +275,7 @@ gfc_finish (void) > > > gfc_cpp_done (); > > > gfc_done_1 (); > > > gfc_release_include_path (); > > > - return; > > > > namely here? > > > > > + gfc_cpp_free_cpp_dirs (); > > > } > > > > Why not call free_cpp_dirs () here directly, omit all unnecessary > > stuff, and maybe only add a brief comment here? > > cpp.c includes incpath.h, f95-lang.c does not and should not. > So the cleanest thing is to keep the cpp handling in cpp.[ch] and have > the language frontend call into it's cpp bits. > > It would be rather rogue to > extern void free_cpp_dirs (void); > in f95-lang.c and directly call it in gfc_finish, i'd say? > > thanks,
diff --git a/gcc/fortran/cpp.c b/gcc/fortran/cpp.c index e86386c8b17..04fe8fe460b 100644 --- a/gcc/fortran/cpp.c +++ b/gcc/fortran/cpp.c @@ -728,12 +728,20 @@ gfc_cpp_done (void) cpp_clear_file_cache (cpp_in); } -/* PATH must be malloc-ed and NULL-terminated. */ +/* Free all cpp include dirs. */ +void +gfc_cpp_free_cpp_dirs (void) +{ + free_cpp_dirs (); +} + +/* PATH must be NULL-terminated. */ void gfc_cpp_add_include_path (char *path, bool user_supplied) { /* CHAIN sets cpp_dir->sysp which differs from 0 if PATH is a system - include path. Fortran does not define any system include paths. */ + include path. Fortran does not define any system include paths. + incpath.c manages the incoming, xstrdup()ed path. */ int cxx_aware = 0; add_path (path, INC_BRACKET, cxx_aware, user_supplied); @@ -742,6 +750,7 @@ gfc_cpp_add_include_path (char *path, bool user_supplied) void gfc_cpp_add_include_path_after (char *path, bool user_supplied) { + /* incpath.c manages the incoming, xstrdup()ed path. */ int cxx_aware = 0; add_path (path, INC_AFTER, cxx_aware, user_supplied); } diff --git a/gcc/fortran/cpp.h b/gcc/fortran/cpp.h index 44644a2a333..963b9a9c89e 100644 --- a/gcc/fortran/cpp.h +++ b/gcc/fortran/cpp.h @@ -46,6 +46,7 @@ void gfc_cpp_post_options (bool); bool gfc_cpp_preprocess (const char *source_file); void gfc_cpp_done (void); +void gfc_cpp_free_cpp_dirs (void); void gfc_cpp_add_include_path (char *path, bool user_supplied); void gfc_cpp_add_include_path_after (char *path, bool user_supplied); diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c index 58dcaf01d75..ec4c2cf01d9 100644 --- a/gcc/fortran/f95-lang.c +++ b/gcc/fortran/f95-lang.c @@ -275,7 +275,7 @@ gfc_finish (void) gfc_cpp_done (); gfc_done_1 (); gfc_release_include_path (); - return; + gfc_cpp_free_cpp_dirs (); } /* These functions and variables deal with binding contours. We only diff --git a/gcc/fortran/scanner.c b/gcc/fortran/scanner.c index 69b81ab97f8..268d1e3e7fb 100644 --- a/gcc/fortran/scanner.c +++ b/gcc/fortran/scanner.c @@ -370,7 +370,7 @@ add_path_to_list (gfc_directorylist **list, const char *path, char *q; size_t len; int i; - + p = path; while (*p == ' ' || *p == '\t') /* someone might do "-I include" */ if (*p++ == '\0') @@ -409,7 +409,7 @@ add_path_to_list (gfc_directorylist **list, const char *path, *list = dir; dir->use_for_modules = use_for_modules; dir->warn = warn; - dir->path = XCNEWVEC (char, strlen (p) + 2); + dir->path = XNEWVEC (char, strlen (p) + 2); strcpy (dir->path, p); strcat (dir->path, "/"); /* make '/' last character */ } @@ -424,8 +424,9 @@ gfc_add_include_path (const char *path, bool use_for_modules, bool file_dir, defer_warn); /* For '#include "..."' these directories are automatically searched. */ + /* CPP manages 'path' on it's own via incpath.c so give it it's own copy. */ if (!file_dir) - gfc_cpp_add_include_path (xstrdup(path), true); + gfc_cpp_add_include_path (xstrdup (path), true); }
From: Bernhard Reutner-Fischer <aldot@gcc.gnu.org> gcc/fortran/ChangeLog: PR fortran/68800 * cpp.h (gfc_cpp_free_cpp_dirs): New declaration. * cpp.c (gfc_cpp_free_cpp_dirs): New definition. (gfc_cpp_add_include_path, gfc_cpp_add_include_path_after): Add comment. * f95-lang.c (gfc_finish): Call gfc_cpp_free_cpp_dirs. * scanner.c (add_path_to_list): Allocate unzeroed memory. (gfc_add_include_path): Add comment and fix formatting. --- Bootstrapped and regtested without regressions on x86_64-unknown-linux. Ok for trunk? Note: in add_path_to_list i changed dir->path = XCNEWVEC to XNEWVEC since strcpy and strcat add a terminating null byte for us anyway. Fixes: -== 68 bytes in 1 blocks are still reachable in loss record 228 of 371 -== at : malloc (vg_replace_malloc.c:380) -== by : xmalloc (xmalloc.c:149) -== by : xstrdup (xstrdup.c:34) -== by : gfc_add_include_path(char const*, bool, bool, bool, bool) (scanner.c :428) -== by : gfc_handle_option(unsigned long, char const*, long, int, unsigned in t, cl_option_handlers const*) (options.c:702) -== by : handle_option(gcc_options*, gcc_options*, cl_decoded_option const*, unsigned int, int, unsigned int, cl_option_handlers const*, bool, diagnostic_con text*) (opts-common.c:1181) -== by : read_cmdline_option(gcc_options*, gcc_options*, cl_decoded_option*, unsigned int, unsigned int, cl_option_handlers const*, diagnostic_context*) (opt s-common.c:1431) -== by : read_cmdline_options (opts-global.c:237) -== by : decode_options(gcc_options*, gcc_options*, cl_decoded_option*, unsig ned int, unsigned int, diagnostic_context*, void (*)()) (opts-global.c:319) -== by : toplev::main(int, char**) (toplev.c:2273) -== by : main (main.c:39) --- gcc/fortran/cpp.c | 13 +++++++++++-- gcc/fortran/cpp.h | 1 + gcc/fortran/f95-lang.c | 2 +- gcc/fortran/scanner.c | 7 ++++--- 4 files changed, 17 insertions(+), 6 deletions(-)