* [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 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
* 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
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).