From: Harald Anlauf <anlauf@gmx.de>
To: gcc-patches@gcc.gnu.org
Cc: fortran@gcc.gnu.org
Subject: Re: [PATCH 2/2] Fortran: Fix memory leak in gfc_add_include_path [PR68800]
Date: Sat, 6 Nov 2021 20:22:53 +0100 [thread overview]
Message-ID: <cfafdd6f-5e1d-4d2c-cb40-bb627c6fe43e@gmx.de> (raw)
In-Reply-To: <20211105211718.2261686-3-rep.dot.nop@gmail.com>
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
WARNING: multiple messages have this Message-ID
From: Harald Anlauf <anlauf@gmx.de>
To: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>,
gcc-patches@gcc.gnu.org, fortran@gcc.gnu.org
Cc: Bernhard Reutner-Fischer <aldot@gcc.gnu.org>
Subject: Re: [PATCH 2/2] Fortran: Fix memory leak in gfc_add_include_path [PR68800]
Date: Sat, 6 Nov 2021 20:22:53 +0100 [thread overview]
Message-ID: <cfafdd6f-5e1d-4d2c-cb40-bb627c6fe43e@gmx.de> (raw)
Message-ID: <20211106192253.2_SEO7y5xFN6ey-WL_l7npsO1Lcob1JfG_4prJtWQHE@z> (raw)
In-Reply-To: <20211105211718.2261686-3-rep.dot.nop@gmail.com>
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
next prev parent reply other threads:[~2021-11-06 19:23 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cfafdd6f-5e1d-4d2c-cb40-bb627c6fe43e@gmx.de \
--to=anlauf@gmx.de \
--cc=fortran@gcc.gnu.org \
--cc=gcc-patches@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).