Message ID | 20170912042325.14927-2-sergiodj@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 77783 invoked by alias); 12 Sep 2017 04:23:36 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 77740 invoked by uid 89); 12 Sep 2017 04:23:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 12 Sep 2017 04:23:34 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 06EB68553F for <gdb-patches@sourceware.org>; Tue, 12 Sep 2017 04:23:33 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 06EB68553F Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=sergiodj@redhat.com Received: from psique.yyz.redhat.com (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTP id DA09E70BC5; Tue, 12 Sep 2017 04:23:30 +0000 (UTC) From: Sergio Durigan Junior <sergiodj@redhat.com> To: GDB Patches <gdb-patches@sourceware.org> Cc: Pedro Alves <palves@redhat.com>, Sergio Durigan Junior <sergiodj@redhat.com> Subject: [PATCH 1/4] Make gdb_dirbuf local to functions Date: Tue, 12 Sep 2017 00:23:22 -0400 Message-Id: <20170912042325.14927-2-sergiodj@redhat.com> In-Reply-To: <20170912042325.14927-1-sergiodj@redhat.com> References: <20170912042325.14927-1-sergiodj@redhat.com> X-IsSubscribed: yes |
Commit Message
Sergio Durigan Junior
Sept. 12, 2017, 4:23 a.m. UTC
This is not a requirement for the feature, but I think it's a good cleanup and is related anyway. Currently we have "current_directory" and "gdb_dirbuf" globals, which means that we basically have two possible places to consult when we want to know GDB's current working directory. This is not ideal and can lead to confusion, so my proposal is to "internalize" the "gdb_dirbuf" variable, because it is really just that: a buffer to be used when calling "getcwd". With this, only "current_directory" should be used to determine the cwd. gdb/ChangeLog: yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com> * cli/cli-cmds.c (quit_command): Declare "gdb_dirbuf". (cd_command): Free "current_directory" before assigning to it. * main.c (captured_main_1): Likewise. Call "xstrdup" when storing it on "current_directory". * mi/mi-cmd-env.c (mi_cmd_env_pwd): Declare "gdb_dirbuf". * top.c (gdb_dirbuf): Remove global declaration. * top.h (gdb_dirbuf): Likewise. --- gdb/cli/cli-cmds.c | 7 ++++++- gdb/main.c | 4 +++- gdb/mi/mi-cmd-env.c | 1 + gdb/top.c | 3 --- gdb/top.h | 1 - 5 files changed, 10 insertions(+), 6 deletions(-)
Comments
On 09/12/2017 05:23 AM, Sergio Durigan Junior wrote: > This is not a requirement for the feature, but I think it's a good > cleanup and is related anyway. Currently we have "current_directory" > and "gdb_dirbuf" globals, which means that we basically have two > possible places to consult when we want to know GDB's current working > directory. > > This is not ideal and can lead to confusion, so my proposal is to > "internalize" the "gdb_dirbuf" variable, because it is really just > that: a buffer to be used when calling "getcwd". With this, only > "current_directory" should be used to determine the cwd. > > gdb/ChangeLog: > yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com> > > * cli/cli-cmds.c (quit_command): Declare "gdb_dirbuf". > (cd_command): Free "current_directory" before assigning to it. > * main.c (captured_main_1): Likewise. Call "xstrdup" when > storing it on "current_directory". > * mi/mi-cmd-env.c (mi_cmd_env_pwd): Declare "gdb_dirbuf". > * top.c (gdb_dirbuf): Remove global declaration. > * top.h (gdb_dirbuf): Likewise. > --- > gdb/cli/cli-cmds.c | 7 ++++++- > gdb/main.c | 4 +++- > gdb/mi/mi-cmd-env.c | 1 + > gdb/top.c | 3 --- > gdb/top.h | 1 - > 5 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c > index 883844ee70..64e893c784 100644 > --- a/gdb/cli/cli-cmds.c > +++ b/gdb/cli/cli-cmds.c > @@ -380,6 +380,8 @@ quit_command (char *args, int from_tty) > static void > pwd_command (char *args, int from_tty) > { > + char gdb_dirbuf[BUFSIZ]; I don't recall offhand -- what's "BUFSIZ"? What defines it and is it guaranteed to be the right size for getcwd? > + > if (args) > error (_("The \"pwd\" command does not take an argument: %s"), args); > if (! getcwd (gdb_dirbuf, sizeof (gdb_dirbuf))) > @@ -434,7 +436,10 @@ cd_command (char *dir, int from_tty) > > dir_holder.reset (savestring (dir, len)); > if (IS_ABSOLUTE_PATH (dir_holder.get ())) > - current_directory = dir_holder.release (); > + { > + xfree (current_directory); > + current_directory = dir_holder.release (); > + } > else > { > if (IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])) > diff --git a/gdb/main.c b/gdb/main.c > index a0646edad6..9837729966 100644 > --- a/gdb/main.c > +++ b/gdb/main.c > @@ -549,10 +549,12 @@ captured_main_1 (struct captured_main_args *context) > (xstrprintf ("%s: warning: ", gdb_program_name)); > warning_pre_print = tmp_warn_preprint.get (); > > + char gdb_dirbuf[BUFSIZ]; > + > if (! getcwd (gdb_dirbuf, sizeof (gdb_dirbuf))) > perror_warning_with_name (_("error finding working directory")); > > - current_directory = gdb_dirbuf; > + current_directory = xstrdup (gdb_dirbuf); As an extension, on GNU/Linux, you can call 'getcwd (NULL, 0)' and that returns a heap-allocated buffer of the necessary size. Can we rely on gnulib to implement that behavior everywhere? Since it seems like we're xstrdup'ing the dir anyway, that likely would simplify things, and remove one arbitrary hardcoded limit, while at it. Thanks, Pedro Alves
On Wednesday, September 13 2017, Pedro Alves wrote: > On 09/12/2017 05:23 AM, Sergio Durigan Junior wrote: >> This is not a requirement for the feature, but I think it's a good >> cleanup and is related anyway. Currently we have "current_directory" >> and "gdb_dirbuf" globals, which means that we basically have two >> possible places to consult when we want to know GDB's current working >> directory. >> >> This is not ideal and can lead to confusion, so my proposal is to >> "internalize" the "gdb_dirbuf" variable, because it is really just >> that: a buffer to be used when calling "getcwd". With this, only >> "current_directory" should be used to determine the cwd. >> >> gdb/ChangeLog: >> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com> >> >> * cli/cli-cmds.c (quit_command): Declare "gdb_dirbuf". >> (cd_command): Free "current_directory" before assigning to it. >> * main.c (captured_main_1): Likewise. Call "xstrdup" when >> storing it on "current_directory". >> * mi/mi-cmd-env.c (mi_cmd_env_pwd): Declare "gdb_dirbuf". >> * top.c (gdb_dirbuf): Remove global declaration. >> * top.h (gdb_dirbuf): Likewise. >> --- >> gdb/cli/cli-cmds.c | 7 ++++++- >> gdb/main.c | 4 +++- >> gdb/mi/mi-cmd-env.c | 1 + >> gdb/top.c | 3 --- >> gdb/top.h | 1 - >> 5 files changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c >> index 883844ee70..64e893c784 100644 >> --- a/gdb/cli/cli-cmds.c >> +++ b/gdb/cli/cli-cmds.c >> @@ -380,6 +380,8 @@ quit_command (char *args, int from_tty) >> static void >> pwd_command (char *args, int from_tty) >> { >> + char gdb_dirbuf[BUFSIZ]; > > I don't recall offhand -- what's "BUFSIZ"? What defines it > and is it guaranteed to be the right size for getcwd? BUFSIZ is defined by stdio.h, and is 8192 on my Fedora 25. This is much greater than the previous "1024" that was being used before. For reproducibility reasons I didn't want to use PATH_MAX. >> + >> if (args) >> error (_("The \"pwd\" command does not take an argument: %s"), args); >> if (! getcwd (gdb_dirbuf, sizeof (gdb_dirbuf))) >> @@ -434,7 +436,10 @@ cd_command (char *dir, int from_tty) >> >> dir_holder.reset (savestring (dir, len)); >> if (IS_ABSOLUTE_PATH (dir_holder.get ())) >> - current_directory = dir_holder.release (); >> + { >> + xfree (current_directory); >> + current_directory = dir_holder.release (); >> + } >> else >> { >> if (IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])) >> diff --git a/gdb/main.c b/gdb/main.c >> index a0646edad6..9837729966 100644 >> --- a/gdb/main.c >> +++ b/gdb/main.c >> @@ -549,10 +549,12 @@ captured_main_1 (struct captured_main_args *context) >> (xstrprintf ("%s: warning: ", gdb_program_name)); >> warning_pre_print = tmp_warn_preprint.get (); >> >> + char gdb_dirbuf[BUFSIZ]; >> + >> if (! getcwd (gdb_dirbuf, sizeof (gdb_dirbuf))) >> perror_warning_with_name (_("error finding working directory")); >> >> - current_directory = gdb_dirbuf; >> + current_directory = xstrdup (gdb_dirbuf); > > As an extension, on GNU/Linux, you can call 'getcwd (NULL, 0)' and > that returns a heap-allocated buffer of the necessary size. Can we > rely on gnulib to implement that behavior everywhere? Since > it seems like we're xstrdup'ing the dir anyway, that likely would > simplify things, and remove one arbitrary hardcoded limit, while > at it. That's true, and it's better when you consider reproducible builds as well. According to gnulib: https://www.gnu.org/software/gnulib/manual/html_node/getcwd.html It is better to rely on this better version of getcwd. Thanks,
On 09/13/2017 11:03 PM, Sergio Durigan Junior wrote: > On Wednesday, September 13 2017, Pedro Alves wrote: >>> --- a/gdb/cli/cli-cmds.c >>> +++ b/gdb/cli/cli-cmds.c >>> @@ -380,6 +380,8 @@ quit_command (char *args, int from_tty) >>> static void >>> pwd_command (char *args, int from_tty) >>> { >>> + char gdb_dirbuf[BUFSIZ]; >> >> I don't recall offhand -- what's "BUFSIZ"? What defines it >> and is it guaranteed to be the right size for getcwd? > > BUFSIZ is defined by stdio.h, and is 8192 on my Fedora 25. This is much > greater than the previous "1024" that was being used before. Ah, that BUFSIZ. That's the FILE* stream buffer size [1], so I don't see it makes sense to use it for paths over PATH_MAX. It happens to be greater that 1024 on your system, but that's not guaranteed, no? [1] - http://pubs.opengroup.org/onlinepubs/9699919799/functions/setbuf.html http://pubs.opengroup.org/onlinepubs/009695399/basedefs/stdio.h.html > For reproducibility reasons I didn't want to use PATH_MAX. I don't understand this. Can you expand? What's not reproducible about PATH_MAX? How's that different from BUFSIZ? >> As an extension, on GNU/Linux, you can call 'getcwd (NULL, 0)' and >> that returns a heap-allocated buffer of the necessary size. Can we >> rely on gnulib to implement that behavior everywhere? Since >> it seems like we're xstrdup'ing the dir anyway, that likely would >> simplify things, and remove one arbitrary hardcoded limit, while >> at it. > > That's true, and it's better when you consider reproducible builds as > well. /me confused about that. > > According to gnulib: > > https://www.gnu.org/software/gnulib/manual/html_node/getcwd.html > > It is better to rely on this better version of getcwd. Alright. This makes the above moot, though I'd still like to understand the reproducibility argument, since I suspect that may come back in other cases. Thanks, Pedro Alves
On Wednesday, September 13 2017, Pedro Alves wrote: > On 09/13/2017 11:03 PM, Sergio Durigan Junior wrote: >> On Wednesday, September 13 2017, Pedro Alves wrote: > >>>> --- a/gdb/cli/cli-cmds.c >>>> +++ b/gdb/cli/cli-cmds.c >>>> @@ -380,6 +380,8 @@ quit_command (char *args, int from_tty) >>>> static void >>>> pwd_command (char *args, int from_tty) >>>> { >>>> + char gdb_dirbuf[BUFSIZ]; >>> >>> I don't recall offhand -- what's "BUFSIZ"? What defines it >>> and is it guaranteed to be the right size for getcwd? >> >> BUFSIZ is defined by stdio.h, and is 8192 on my Fedora 25. This is much >> greater than the previous "1024" that was being used before. > > Ah, that BUFSIZ. That's the FILE* stream buffer size [1], so I don't > see it makes sense to use it for paths over PATH_MAX. It happens to be > greater that 1024 on your system, but that's not guaranteed, no? > > [1] - http://pubs.opengroup.org/onlinepubs/9699919799/functions/setbuf.html > http://pubs.opengroup.org/onlinepubs/009695399/basedefs/stdio.h.html AFAIK that's not a guarantee, indeed. Although "1024" is also a very low value for path names on some systems (for example, here PATH_MAX is defined as 4096), we already had this problem before. >> For reproducibility reasons I didn't want to use PATH_MAX. > > I don't understand this. Can you expand? What's not reproducible > about PATH_MAX? How's that different from BUFSIZ? > >>> As an extension, on GNU/Linux, you can call 'getcwd (NULL, 0)' and >>> that returns a heap-allocated buffer of the necessary size. Can we >>> rely on gnulib to implement that behavior everywhere? Since >>> it seems like we're xstrdup'ing the dir anyway, that likely would >>> simplify things, and remove one arbitrary hardcoded limit, while >>> at it. >> >> That's true, and it's better when you consider reproducible builds as >> well. > > /me confused about that. > >> >> According to gnulib: >> >> https://www.gnu.org/software/gnulib/manual/html_node/getcwd.html >> >> It is better to rely on this better version of getcwd. > > Alright. This makes the above moot, though I'd still like to > understand the reproducibility argument, since I suspect that > may come back in other cases. Well, maybe this is more a question of portability than reproducibility, but I mentioned the latter because I remember doing some fixes in some Debian packages that were failing the reproducible tests due to the presence of PATH_MAX. They were actually failing to build on certain targets, but that can be seen as non-reproducibility as well. PATH_MAX is defined by POSIX but is not used by every system. GNU/Hurd, for example, doesn't define it. As usual with GNU programs the "correct" thing to do is not to rely on some hard-coded limit imposed by a header file, but rather to make use of the fact that glibc's getcwd offers the possibility to allocate the variable that holds the path according to its actual size. There are some other problems with PATH_MAX, e.g., the fact that the maximum length of a pathname is really defined by the underlying filesystem you're using, so just using "4096" doesn't really reflect the reality. Again, maybe I should have said "portability issues", but I consider them reproducibility issues as well. Thanks,
On 09/13/2017 11:46 PM, Sergio Durigan Junior wrote: >> > Ah, that BUFSIZ. That's the FILE* stream buffer size [1], so I don't >> > see it makes sense to use it for paths over PATH_MAX. It happens to be >> > greater that 1024 on your system, but that's not guaranteed, no? >> > >> > [1] - http://pubs.opengroup.org/onlinepubs/9699919799/functions/setbuf.html >> > http://pubs.opengroup.org/onlinepubs/009695399/basedefs/stdio.h.html > AFAIK that's not a guarantee, indeed. Although "1024" is also a very > low value for path names on some systems (for example, here PATH_MAX is > defined as 4096), we already had this problem before. Yes, but at least 1024 was stable and usually good enough. >>> For reproducibility reasons I didn't want to use PATH_MAX. >> >> I don't understand this. Can you expand? What's not reproducible >> about PATH_MAX? How's that different from BUFSIZ? >> >>>> As an extension, on GNU/Linux, you can call 'getcwd (NULL, 0)' and >>>> that returns a heap-allocated buffer of the necessary size. Can we >>>> rely on gnulib to implement that behavior everywhere? Since >>>> it seems like we're xstrdup'ing the dir anyway, that likely would >>>> simplify things, and remove one arbitrary hardcoded limit, while >>>> at it. >>> >>> That's true, and it's better when you consider reproducible builds as >>> well. >> >> /me confused about that. >> >>> >>> According to gnulib: >>> >>> https://www.gnu.org/software/gnulib/manual/html_node/getcwd.html >>> >>> It is better to rely on this better version of getcwd. >> >> Alright. This makes the above moot, though I'd still like to >> understand the reproducibility argument, since I suspect that >> may come back in other cases. > > Well, maybe this is more a question of portability than reproducibility, > but I mentioned the latter because I remember doing some fixes in some > Debian packages that were failing the reproducible tests due to the > presence of PATH_MAX. They were actually failing to build on certain > targets, but that can be seen as non-reproducibility as well. I think that's stretching what reproducibility means too far, and ends up being confusing. AFAICT, it'd be like saying that a program isn't reproducible because it uses "ptrace", because "ptrace" isn't portable. Well, that "ptrace" call may be the whole point of the program. If you can always build the program on environment A and end up with the same binary, then the program is reproducible on environment A. That does not mean that you should expect to build the program under some environment B [e.g., different compiler version or different system libraries, or different kernel, or different architecture, or ...] and get the same binary as in environment A. > > PATH_MAX is defined by POSIX but is not used by every system. GNU/Hurd, > for example, doesn't define it. As usual with GNU programs the > "correct" thing to do is not to rely on some hard-coded limit imposed by > a header file, but rather to make use of the fact that glibc's getcwd > offers the possibility to allocate the variable that holds the path > according to its actual size. Correct. That's exactly why I thought of suggesting getcwd's extension. > > There are some other problems with PATH_MAX, e.g., the fact that the > maximum length of a pathname is really defined by the underlying > filesystem you're using, so just using "4096" doesn't really reflect the > reality. Right, on GNU/Linux we can certainly have paths longer than that. > > Again, maybe I should have said "portability issues", but I consider > them reproducibility issues as well. Alright, glad we cleared this up, and glad that it's all moot now. :-) Thanks, Pedro Alves
diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c index 883844ee70..64e893c784 100644 --- a/gdb/cli/cli-cmds.c +++ b/gdb/cli/cli-cmds.c @@ -380,6 +380,8 @@ quit_command (char *args, int from_tty) static void pwd_command (char *args, int from_tty) { + char gdb_dirbuf[BUFSIZ]; + if (args) error (_("The \"pwd\" command does not take an argument: %s"), args); if (! getcwd (gdb_dirbuf, sizeof (gdb_dirbuf))) @@ -434,7 +436,10 @@ cd_command (char *dir, int from_tty) dir_holder.reset (savestring (dir, len)); if (IS_ABSOLUTE_PATH (dir_holder.get ())) - current_directory = dir_holder.release (); + { + xfree (current_directory); + current_directory = dir_holder.release (); + } else { if (IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])) diff --git a/gdb/main.c b/gdb/main.c index a0646edad6..9837729966 100644 --- a/gdb/main.c +++ b/gdb/main.c @@ -549,10 +549,12 @@ captured_main_1 (struct captured_main_args *context) (xstrprintf ("%s: warning: ", gdb_program_name)); warning_pre_print = tmp_warn_preprint.get (); + char gdb_dirbuf[BUFSIZ]; + if (! getcwd (gdb_dirbuf, sizeof (gdb_dirbuf))) perror_warning_with_name (_("error finding working directory")); - current_directory = gdb_dirbuf; + current_directory = xstrdup (gdb_dirbuf); /* Set the sysroot path. */ gdb_sysroot = relocate_gdb_directory (TARGET_SYSTEM_ROOT, diff --git a/gdb/mi/mi-cmd-env.c b/gdb/mi/mi-cmd-env.c index 977b6e274d..55d08ee5f2 100644 --- a/gdb/mi/mi-cmd-env.c +++ b/gdb/mi/mi-cmd-env.c @@ -62,6 +62,7 @@ void mi_cmd_env_pwd (const char *command, char **argv, int argc) { struct ui_out *uiout = current_uiout; + char gdb_dirbuf[BUFSIZ]; if (argc > 0) error (_("-environment-pwd: No arguments allowed")); diff --git a/gdb/top.c b/gdb/top.c index 742c1e7a07..0f36dce760 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -133,9 +133,6 @@ show_confirm (struct ui_file *file, int from_tty, char *current_directory; -/* The directory name is actually stored here (usually). */ -char gdb_dirbuf[1024]; - /* The last command line executed on the console. Used for command repetitions. */ char *saved_command_line; diff --git a/gdb/top.h b/gdb/top.h index 45798897f6..6b66083995 100644 --- a/gdb/top.h +++ b/gdb/top.h @@ -219,7 +219,6 @@ extern void ui_unregister_input_event_handler (struct ui *ui); /* From top.c. */ extern char *saved_command_line; extern int confirm; -extern char gdb_dirbuf[1024]; extern int inhibit_gdbinit; extern const char gdbinit[];