From patchwork Sat Sep 7 23:50:59 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 34446 Received: (qmail 74685 invoked by alias); 7 Sep 2019 23:51:06 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 74676 invoked by uid 89); 7 Sep 2019 23:51:06 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.2 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=locate, H*RU:sk:host86-, producer, acceptance X-HELO: mail-wm1-f52.google.com Received: from mail-wm1-f52.google.com (HELO mail-wm1-f52.google.com) (209.85.128.52) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 07 Sep 2019 23:51:03 +0000 Received: by mail-wm1-f52.google.com with SMTP id y135so10046066wmc.1 for ; Sat, 07 Sep 2019 16:51:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=YI91gnypC9RwNlVNaN7c1Eat0DHXRLUT4kiTZevcVH0=; b=PJmbie+CkPyPj14m4IzpcqXUuc8gVZLiiz14xJKVLJfXIk3O8R2xzE7Xn/iYm91AOK gewuzW6XrnDLpOgHNY0b3RAVVK/YoiadwIdAzAdXV+wjKdPQLE/liytPBL5WvQ5vYei/ kl0Fmll9FGyhDcKvP6PH8V8pzfocKVfPotyUuVgReUeQ4Owa5pvKKVMi2+VVFG8HScwl zNadkr6/QrGzpU69WnBZ1nto7LNEUoJ0Q8IBDr0DNsfxKB9K9V1fXLDH4DtLX+u+7R23 knvMvV9Qq++c+gxOD2FRopdmehoJeF7tIxeteAQY53pJhAo7+9NT/TezGHhkWQywJ51p flHQ== Return-Path: Received: from localhost (host86-161-16-231.range86-161.btcentralplus.com. [86.161.16.231]) by smtp.gmail.com with ESMTPSA id f197sm9743326wme.22.2019.09.07.16.50.59 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 07 Sep 2019 16:51:00 -0700 (PDT) Date: Sun, 8 Sep 2019 00:50:59 +0100 From: Andrew Burgess To: Mike Gulick Cc: "gdb-patches@sourceware.org" Subject: Re: [RFC] Apply compilation dir to source_path lookup Message-ID: <20190907235059.GN6076@embecosm.com> References: <8058b501-9020-84f1-ecf4-b46bfe3b86e3@mathworks.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <8058b501-9020-84f1-ecf4-b46bfe3b86e3@mathworks.com> X-Fortune: A subversive is anyone who can out-argue their government. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes * Mike Gulick [2019-09-05 22:40:48 +0000]: > Hi, > > I'm having trouble getting gdb to find the corresponding source for a > binary whose compilation directory was rewritten using > -fdebug-prefix-map. I am doing this in order to make builds > reproducible. I am using this gcc switch to remove a portion of the > compile directory, e.g. > > $ cd $HOME/test/src/ > $ gcc -g -o test -fdebug-prefix-map=$HOME= test.c > $ dwarfdump test > ... > DW_AT_name test.c > DW_AT_comp_dir /test/src > ... > > When attempting to debug the resulting binary, gdb is unable to find the > source file even if I add the removed path $HOME to the gdb source > path: > > $ cd $HOME > $ gdb --nh ~/test/src/test > GNU gdb (GDB) 8.3 > ... > (gdb) directory /home/mgulick > Source directories searched: /home/mgulick:$cdir:$cwd > (gdb) info sources > Source files for which symbols have been read in: > > /test/src/test.c > > Source files for which symbols will be read in on demand: > > > (gdb) b test.c:3 > Breakpoint 1 at 0x664: file test.c, line 3. > (gdb) r > Starting program: /mathworks/home/mgulick/test/src/test > > Breakpoint 1, main () at test.c:3 > 3 test.c: No such file or directory. > > > It turns out that only the source file, i.e. DW_AT_name, is used when > searching the source path. This is surprising given the example with > '/usr/src/foo-1.0/lib/foo.c' and '/mnt/cross' in the gdb documentation > here: https://sourceware.org/gdb/onlinedocs/gdb/Source-Path.html. It > seems uncommon for DW_AT_name to be the full path to the source file in > most programs I have come across. In that example from the > documentation, it is likely that DW_AT_name would be 'lib/foo.c' and > DW_AT_comp_dir would be '/usr/src/foo-1.0'. That depends on how you compile your source: gcc -o my.exe /usr/src/foo-1.0/lib/foo.c would result in DW_AT_name being '/usr/src/foo-1.0/lib/foo.c' I believe. So I think the example is fine, though I don't think it really goes into enough details. > > I have implemented two different approaches to handle this, both of > which work, and I wanted feedback on a) whether you think this is a > legitimate bug/feature, and b) which approach you prefer. > > Approach 1 is to include another pass in find_and_open_source where the > compilation directory is prepended to the filename before searching for > the file with openp. This allows the example I provided above to work > as-is. > > Approach 2 is to allow '$cdir' to appear as part of an entry in the > source_path. Currently it is only allowed to exist as a standalone > entry. This would allow me to say 'directory /home/mgulick/$cdir', and > find_and_open_source would expand the $cdir part of this string to the > compilation directory. > > I prefer approach 1 for a couple of reasons: > > 1) It is simpler for users to understand. It doesn't really require > understanding the difference between DW_AT_comp_dir and DW_AT_name. It > will match the source file in /home/mgulick/test.c, /test/src/test.c, or > /home/mgulick/test/src/test.c. Of course, since '$cdir' is still in > source_path, it will also look in /test/src/test/src/test.c, which is a > little confusing, but shouldn't be a problem (and we could explicitly > remove '$cdir' from the path when doing this search). > > 2) It seems to match the existing gdb documentation. > > I dislike approach 2 because it makes the directory strings more > complicated, and it also means I'm unable to have a directory named > '/home/mgulick/$cdir' (bizarre, I know, but such things are > possible). I agree with your approach, I think it's nicer if things can "just work" for the user, and this seems like a good approach. > > Here is a preliminary patch for the first approach. I will go ahead and > add a test-case and re-send a formal patch review request if this seems > like an acceptable solution. Do you have a copyright assignment in place? Any patch over a couple of trivial lines requires an assignment. Others might correct me, but this patch would seem to be big enough to me. > > Thanks, > Mike > > diff --git a/gdb/source.c b/gdb/source.c > index b27f210802..b7b5741028 100644 > --- a/gdb/source.c > +++ b/gdb/source.c > @@ -1033,7 +1033,35 @@ find_and_open_source (const char *filename, > openp_flags flags = OPF_SEARCH_IN_PATH; > if (basenames_may_differ) > flags |= OPF_RETURN_REALPATH; > + > + /* Try to locate file using filename */ Comments need to end with '.', and there should always be two spaces after a '.' in comments. > result = openp (path, flags, filename, OPEN_MODE, fullname); > + if (result < 0 && dirname != NULL) > + { > + /* Try to locate file using compilation dir + filename. This is helpful > + if part of the compilation directory was removed, e.g. using gcc's > + -fdebug-prefix-map, and we have added the missing prefix to > + source_path. */ > + /* Allocate space for dirname, possibly an extra slash, the filename, > + and terminating null */ You need two spaces at the end of your comments, and can you fold these two comments into one. > + char * cdir_filename = (char *) > + alloca (strlen (dirname) + strlen(SLASH_STRING) + strlen (filename) + 1); Whitespace between function name and opening parenthesis. > + > + cdir_filename = strcpy (cdir_filename, dirname); > + int len = strlen (cdir_filename); /* last char in cdir_filename */ Comments should be formatted as sentences, so capital 'L' here, and '. ' and the end. > + > + /* Add directory separator if not provided by dirname or filename */ > + if (!(IS_DIR_SEPARATOR (cdir_filename[len]) || > + IS_DIR_SEPARATOR (filename[0]))) Break the line before the '||'. > + strcat (cdir_filename, SLASH_STRING); > + else if (IS_DIR_SEPARATOR (cdir_filename[len]) && > + IS_DIR_SEPARATOR (filename[0])) Same, line break before '&&'. > + /* Both provide a slash, use only one */ Comment formatting. > + cdir_filename[len] = '\0'; > + strcat(cdir_filename,filename); Whitespace before opening parenthesis. > + > + result = openp(path, flags, cdir_filename, OPEN_MODE, fullname); And again. > + } > if (result < 0) > { > /* Didn't work. Try using just the basename. */ You'll also need to write a short ChangeLog entry for the patch. I also wonder if the function could be simplified by using std::string to build the new string rather than all of the strlen/strcpy/strcat calls? Though I don't think that would be a requirement for acceptance. You might also consider extending the documentation relating to source path searching to better describe the process - as you discuss above, the existing documentation is not that clear on the subject. Finally, you'd probably want to write a test.... however, I was wondering how a test for this might be written, and ended up writing one myself. Feel free to use this as a starting point, or just include this as is if it helps. Thanks, Andrew --- commit c10a25ca9a1eb8f7e2cbab9b8fd652fe9feb16de Author: Andrew Burgess Date: Sun Sep 8 00:19:48 2019 +0100 gdb/testsuite: New test for mapped compilation directory Ensure GDB can find source files when compiling using the -fdebug-prefix-map GCC option. The problem is that the compilation directory is not tried against other directories in the directory search path. gdb/testsuite/ChangeLog: * gdb.base/source-dir.c: New file. * gdb.base/source-dir.exp: Add extra test for mapped compilation directory. diff --git a/gdb/testsuite/gdb.base/source-dir.c b/gdb/testsuite/gdb.base/source-dir.c new file mode 100644 index 00000000000..a9bce4a1445 --- /dev/null +++ b/gdb/testsuite/gdb.base/source-dir.c @@ -0,0 +1,5 @@ +int +main () +{ + return 0; +} diff --git a/gdb/testsuite/gdb.base/source-dir.exp b/gdb/testsuite/gdb.base/source-dir.exp index 048c0e95161..ecfb74ef27d 100644 --- a/gdb/testsuite/gdb.base/source-dir.exp +++ b/gdb/testsuite/gdb.base/source-dir.exp @@ -15,9 +15,127 @@ standard_testfile -gdb_start +proc search_dir_list { dirs } { + set output "\r\nSource directories searched: " + append output [join $dirs "\[:;\]"] -set foo "/nOtExStInG" + return ${output} +} -gdb_test "directory $foo/a $foo/b $foo/c" "\r\nSource directories searched: $foo/a\[:;\]$foo/b\[:;\]$foo/c\[:;\]\\\$cdir\[:;\]\\\$cwd" -gdb_test "directory $foo/b $foo/d $foo/c" "\r\nSource directories searched: $foo/b\[:;\]$foo/d\[:;\]$foo/c\[:;\]$foo/a\[:;\]\\\$cdir\[:;\]\\\$cwd" + +# Check that adding directories to the search path changes the order +# in which directories are searched. +proc test_changing_search_directory {} { + gdb_start + + set foo "/nOtExStInG" + + gdb_test "directory $foo/a $foo/b $foo/c" \ + [search_dir_list [list \ + "$foo/a" \ + "$foo/b" \ + "$foo/c" \ + "\\\$cdir" \ + "\\\$cwd"]] + gdb_test "directory $foo/b $foo/d $foo/c" \ + [search_dir_list [list \ + "$foo/b" \ + "$foo/d" \ + "$foo/c" \ + "$foo/a" \ + "\\\$cdir" \ + "\\\$cwd"]] + gdb_exit +} + +# Test that the compilation directory can also be extended with a +# prefix from the directory search path in order to find source files. +proc test_truncated_comp_dir {} { + global srcfile srcdir subdir binfile + + # When we run this test the current directory will be something + # like this: + # /some/path/to/gdb/build/testsuite/ + # We are going to copy the source file out of the source tree into + # a location like this: + # /some/path/to/gdb/build/testsuite/output/gdb.base/soure-dir/ + # + # We will then switch to this directory and compile the source + # file, however, we will ask GCC to remove this prefix from the + # compilation directory in the debug info: + # /some/path/to/gdb/build/testsuite/output/ + # + # As a result the debug information will look like this: + # + # DW_AT_name : source-dir.c + # DW_AT_comp_dir : /gdb.base/source-dir + # + # Finally we switch back to this directory: + # /some/path/to/gdb/build/testsuite/ + # + # and start GDB. There was a time when GDB would be unable to + # find the source file no matter what we added to the directory + # search path, this should now be fixed. + + set original_dir [pwd] + set working_dir [standard_output_file ""] + cd ${working_dir} + + set strip_dir [file normalize "${working_dir}/../.."] + + set new_srcfile [standard_output_file ${srcfile}] + remote_exec host "cp ${srcdir}/${subdir}/${srcfile} ${new_srcfile}" + + set options \ + "debug additional_flags=-fdebug-prefix-map=${strip_dir}=" + if { [gdb_compile "${srcfile}" "${binfile}" \ + executable ${options}] != "" } { + untested "failed to compile" + return -1 + } + + cd ${original_dir} + + clean_restart ${binfile} + + gdb_test_no_output "set directories \$cdir:\$cwd" + gdb_test "show directories" \ + "\r\nSource directories searched: \\\$cdir\[:;\]\\\$cwd" + + if ![runto_main] then { + fail "can't run to main" + return 0 + } + + gdb_test "info source" \ + [multi_line \ + "Current source file is ${srcfile}" \ + "Compilation directory is \[^\n\r\]+" \ + "${srcfile}: No such file or directory."] + + gdb_test "dir $strip_dir" \ + [search_dir_list [list \ + "$strip_dir" \ + "\\\$cdir" \ + "\\\$cwd"]] + gdb_test "list" [multi_line \ + "1\[ \t\]+int" \ + "2\[ \t\]+main \\(\\)" \ + "3\[ \t\]+\\{" \ + "4\[ \t\]+return 0;" \ + "5\[ \t\]+\\}" ] + + gdb_test "info source" \ + [multi_line \ + "Current source file is ${srcfile}" \ + "Compilation directory is \[^\n\r\]+" \ + "Located in ${new_srcfile}" \ + "Contains 5 lines." \ + "Source language is c." \ + "Producer is \[^\n\r\]+" \ + "\[^\n\r\]+" \ + "\[^\n\r\]+" ] +} + +test_changing_search_directory +test_truncated_comp_dir