* [PATCH 0/2] incpath, Fortran: Fix memory leak in gfc_add_include_path @ 2021-11-05 21:17 Bernhard Reutner-Fischer 2021-11-05 21:17 ` [PATCH 1/2] Add free_cpp_dirs() Bernhard Reutner-Fischer ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Bernhard Reutner-Fischer @ 2021-11-05 21:17 UTC (permalink / raw) To: gcc-patches, fortran; +Cc: Bernhard Reutner-Fischer Hi! In order to fix this very minor leak, we need a knob to free all cpp_dirs that were added. This adds a free_cpp_dirs() to gcc/incpath and needs review from some global maintainer or maybe libcpp maintainer for this hunk. Bootstrapped and regtested on x86_64-unknown-linux without regressions. Ok for tunk? Bernhard Reutner-Fischer (2): Add free_cpp_dirs() Fortran: Fix memory leak in gfc_add_include_path [PR68800] gcc/fortran/cpp.c | 13 +++++++++++-- gcc/fortran/cpp.h | 1 + gcc/fortran/f95-lang.c | 2 +- gcc/fortran/scanner.c | 7 ++++--- gcc/incpath.c | 13 +++++++++++++ gcc/incpath.h | 1 + 6 files changed, 31 insertions(+), 6 deletions(-) -- 2.33.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] Add free_cpp_dirs() 2021-11-05 21:17 [PATCH 0/2] incpath, Fortran: Fix memory leak in gfc_add_include_path Bernhard Reutner-Fischer @ 2021-11-05 21:17 ` Bernhard Reutner-Fischer 2021-11-05 21:17 ` [PATCH 2/2] Fortran: Fix memory leak in gfc_add_include_path [PR68800] Bernhard Reutner-Fischer 2021-11-05 22:59 ` [PATCH 0/2] incpath, Fortran: Fix memory leak in gfc_add_include_path Bernhard Reutner-Fischer 2 siblings, 0 replies; 8+ messages in thread From: Bernhard Reutner-Fischer @ 2021-11-05 21:17 UTC (permalink / raw) To: gcc-patches, fortran; +Cc: Bernhard Reutner-Fischer, Bernhard Reutner-Fischer From: Bernhard Reutner-Fischer <aldot@gcc.gnu.org> gcc/ChangeLog: * incpath.c (free_cpp_dirs): New function. * incpath.h (free_cpp_dirs): Ditto. --- This adds a helper to allow the fortran FE to free it's include dirs. Bootstrapped and regtested without new regressions on x86_64-unknown-linux. Ok for trunk? --- gcc/incpath.c | 13 +++++++++++++ gcc/incpath.h | 1 + 2 files changed, 14 insertions(+) diff --git a/gcc/incpath.c b/gcc/incpath.c index 52dbb806b1b..de77a56062c 100644 --- a/gcc/incpath.c +++ b/gcc/incpath.c @@ -513,6 +513,19 @@ get_added_cpp_dirs (incpath_kind chain) return heads[chain]; } +/* Free all cpp dirs. */ +void +free_cpp_dirs (void) +{ + struct cpp_dir **pcur, *cur; + for (pcur = &heads[INC_QUOTE]; *pcur;) + { + cur = *pcur; + *pcur = cur->next; + free_path (cur, REASON_QUIET); + } +} + #if !(defined TARGET_EXTRA_INCLUDES) || !(defined TARGET_EXTRA_PRE_INCLUDES) static void hook_void_charptr_charptr_int (const char *sysroot ATTRIBUTE_UNUSED, const char *iprefix ATTRIBUTE_UNUSED, diff --git a/gcc/incpath.h b/gcc/incpath.h index 2a4a314d7f9..4bf878b4e74 100644 --- a/gcc/incpath.h +++ b/gcc/incpath.h @@ -34,6 +34,7 @@ extern void register_include_chains (cpp_reader *, const char *, int, int, int); extern void add_cpp_dir_path (struct cpp_dir *, incpath_kind); extern struct cpp_dir *get_added_cpp_dirs (incpath_kind); +extern void free_cpp_dirs(void); struct target_c_incpath_s { /* Do extra includes processing. STDINC is false iff -nostdinc was given. */ -- 2.33.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] Fortran: Fix memory leak in gfc_add_include_path [PR68800] 2021-11-05 21:17 [PATCH 0/2] incpath, Fortran: Fix memory leak in gfc_add_include_path Bernhard Reutner-Fischer 2021-11-05 21:17 ` [PATCH 1/2] Add free_cpp_dirs() Bernhard Reutner-Fischer @ 2021-11-05 21:17 ` Bernhard Reutner-Fischer 2021-11-06 19:22 ` Harald Anlauf 2021-11-05 22:59 ` [PATCH 0/2] incpath, Fortran: Fix memory leak in gfc_add_include_path Bernhard Reutner-Fischer 2 siblings, 1 reply; 8+ messages in thread From: Bernhard Reutner-Fischer @ 2021-11-05 21:17 UTC (permalink / raw) To: gcc-patches, fortran; +Cc: Bernhard Reutner-Fischer, Bernhard Reutner-Fischer 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); } -/* 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); } -- 2.33.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Fortran: Fix memory leak in gfc_add_include_path [PR68800] 2021-11-05 21:17 ` [PATCH 2/2] Fortran: Fix memory leak in gfc_add_include_path [PR68800] Bernhard Reutner-Fischer @ 2021-11-06 19:22 ` Harald Anlauf 2021-11-06 19:22 ` Harald Anlauf 2021-11-07 1:38 ` Bernhard Reutner-Fischer 0 siblings, 2 replies; 8+ messages in thread From: Harald Anlauf @ 2021-11-06 19:22 UTC (permalink / raw) To: gcc-patches; +Cc: fortran 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Fortran: Fix memory leak in gfc_add_include_path [PR68800] 2021-11-06 19:22 ` Harald Anlauf @ 2021-11-06 19:22 ` Harald Anlauf 2021-11-07 1:38 ` Bernhard Reutner-Fischer 1 sibling, 0 replies; 8+ messages in thread From: Harald Anlauf @ 2021-11-06 19:22 UTC (permalink / raw) To: Bernhard Reutner-Fischer, gcc-patches, fortran; +Cc: Bernhard Reutner-Fischer 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Fortran: Fix memory leak in gfc_add_include_path [PR68800] 2021-11-06 19:22 ` Harald Anlauf 2021-11-06 19:22 ` Harald Anlauf @ 2021-11-07 1:38 ` Bernhard Reutner-Fischer 2023-04-02 19:27 ` Bernhard Reutner-Fischer 1 sibling, 1 reply; 8+ messages in thread From: Bernhard Reutner-Fischer @ 2021-11-07 1:38 UTC (permalink / raw) To: Harald Anlauf; +Cc: rep.dot.nop, gcc-patches, fortran, Bernhard Reutner-Fischer 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, ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Fortran: Fix memory leak in gfc_add_include_path [PR68800] 2021-11-07 1:38 ` Bernhard Reutner-Fischer @ 2023-04-02 19:27 ` Bernhard Reutner-Fischer 0 siblings, 0 replies; 8+ messages in thread From: Bernhard Reutner-Fischer @ 2023-04-02 19:27 UTC (permalink / raw) To: fortran Cc: rep.dot.nop, Harald Anlauf, gcc-patches, Bernhard Reutner-Fischer, nathan, dmalcolm 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, ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] incpath, Fortran: Fix memory leak in gfc_add_include_path 2021-11-05 21:17 [PATCH 0/2] incpath, Fortran: Fix memory leak in gfc_add_include_path Bernhard Reutner-Fischer 2021-11-05 21:17 ` [PATCH 1/2] Add free_cpp_dirs() Bernhard Reutner-Fischer 2021-11-05 21:17 ` [PATCH 2/2] Fortran: Fix memory leak in gfc_add_include_path [PR68800] Bernhard Reutner-Fischer @ 2021-11-05 22:59 ` Bernhard Reutner-Fischer 2 siblings, 0 replies; 8+ messages in thread From: Bernhard Reutner-Fischer @ 2021-11-05 22:59 UTC (permalink / raw) To: fortran; +Cc: rep.dot.nop, gcc-patches On Fri, 5 Nov 2021 22:17:16 +0100 Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote: > Hi! > > In order to fix this very minor leak, we need a knob to free all > cpp_dirs that were added. > This adds a free_cpp_dirs() to gcc/incpath and needs review from some > global maintainer or maybe libcpp maintainer for this hunk. One thing that i want to note is that it sounds a tiny bit odd that we (the fortran FE) pull in the usual C paths, but maybe that's deliberate and agreed on? We have in incpath.c: > /* Exported function to handle include chain merging, duplicate > removal, and registration with cpplib. */ > void > register_include_chains (cpp_reader *pfile, const char *sysroot, > const char *iprefix, const char *imultilib, > int stdinc, int cxx_stdinc, int verbose) > { > static const char *const lang_env_vars[] = > { "C_INCLUDE_PATH", "CPLUS_INCLUDE_PATH", > "OBJC_INCLUDE_PATH", "OBJCPLUS_INCLUDE_PATH" }; > cpp_options *cpp_opts = cpp_get_options (pfile); > size_t idx = (cpp_opts->objc ? 2: 0); > > if (cpp_opts->cplusplus) > idx++; > else > cxx_stdinc = false; > > /* CPATH and language-dependent environment variables may add to the > include chain. */ > add_env_var_paths ("CPATH", INC_BRACKET); > add_env_var_paths (lang_env_vars[idx], INC_SYSTEM); > > target_c_incpath.extra_pre_includes (sysroot, iprefix, stdinc); where we pass cxx_stdinc=0 and cpp_pts->cplusplus==0 (and nobody complains anout that chatty cxx_stdinc=false and not =0 but so be it) I.e. we do honor both CPATH as well as C_INCLUDE_PATH from the env for fortran preprocessing. Maybe we should document this fact somehow as i do not think we want to change it? [ in gcc/doc/cppenv.texi: @itemx OBJC_INCLUDE_PATH @c Commented out until ObjC++ is part of GCC: @c @itemx OBJCPLUS_INCLUDE_PATH where i think objc++ is currently (still) in the tree so maybe someone may want to tweak the docs here. But i digress.. ] gcc/ada/gnat_ugn.texi documents caveats for globally set C_INCLUDE_PATH when using more than one compiler. [It's the only user of :envvar:`` in the tree, everybody else seems to use @env{} btw.] I cannot judge if ada is aware of pulling in CPATH or if maybe they somehow prevent this, didn't look. Thoughts on the env vars for fortran preprocessing? thanks, > > Bootstrapped and regtested on x86_64-unknown-linux without regressions. > Ok for tunk? > > Bernhard Reutner-Fischer (2): > Add free_cpp_dirs() > Fortran: Fix memory leak in gfc_add_include_path [PR68800] > > gcc/fortran/cpp.c | 13 +++++++++++-- > gcc/fortran/cpp.h | 1 + > gcc/fortran/f95-lang.c | 2 +- > gcc/fortran/scanner.c | 7 ++++--- > gcc/incpath.c | 13 +++++++++++++ > gcc/incpath.h | 1 + > 6 files changed, 31 insertions(+), 6 deletions(-) > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-04-02 19:27 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-05 21:17 [PATCH 0/2] incpath, Fortran: Fix memory leak in gfc_add_include_path Bernhard Reutner-Fischer 2021-11-05 21:17 ` [PATCH 1/2] Add free_cpp_dirs() Bernhard Reutner-Fischer 2021-11-05 21:17 ` [PATCH 2/2] Fortran: Fix memory leak in gfc_add_include_path [PR68800] Bernhard Reutner-Fischer 2021-11-06 19:22 ` Harald Anlauf 2021-11-06 19:22 ` Harald Anlauf 2021-11-07 1:38 ` Bernhard Reutner-Fischer 2023-04-02 19:27 ` Bernhard Reutner-Fischer 2021-11-05 22:59 ` [PATCH 0/2] incpath, Fortran: Fix memory leak in gfc_add_include_path Bernhard Reutner-Fischer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).