From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 128170 invoked by alias); 27 Jul 2018 14:01:55 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 128160 invoked by uid 89); 27 Jul 2018 14:01:55 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_1,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 27 Jul 2018 14:01:53 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id w6RE1kjE002096 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Fri, 27 Jul 2018 10:01:51 -0400 Received: by simark.ca (Postfix, from userid 112) id 7837B1EF29; Fri, 27 Jul 2018 10:01:46 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id CB1D11E059; Fri, 27 Jul 2018 10:01:43 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Fri, 27 Jul 2018 14:01:00 -0000 From: Simon Marchi To: Eli Zaretskii Cc: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH v2 3/4] Add DWARF index cache In-Reply-To: <83o9etrqsj.fsf@gnu.org> References: <1532558824-829-1-git-send-email-simon.marchi@ericsson.com> <1532558824-829-4-git-send-email-simon.marchi@ericsson.com> <83o9etrqsj.fsf@gnu.org> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.6 X-IsSubscribed: yes X-SW-Source: 2018-07/txt/msg00734.txt.bz2 On 2018-07-27 05:06, Eli Zaretskii wrote: >> From: Simon Marchi >> CC: Simon Marchi >> Date: Wed, 25 Jul 2018 18:47:03 -0400 >> >> GDB can generate indexes for DWARF debug information, which, when >> integrated in the original binary, can speed up loading object files. >> This can be done using the gdb-add-index script or directly by the >> linker itself. > > The information in the last sentence is not mentioned in the > documentation patch you provided. I think we should mention that. I think this is already well covered by the rest of the "18.5 Index Files Speed Up GDB" section, just before the content I added, isn't it? >> To help make using the DWARF index more transparent, this patch >> introduces a DWARF index cache. When enabled, loading an index-less >> binary in GDB will automatically save an index file in ~/.cache/gdb. >> When loading that same object file again, the index file will be >> looked >> up and used to load the DWARF index. You therefore get the benefit of >> the DWARF index without having to do additional manual steps or >> modifying your build system. When an index section is already present >> in the file, GDB will prefer that one over looking up the cache. > > Does GDB check whether the cache became stale, i.e. the program was > modified since the last save? This should be documented. And if it > does check, what happens if the cache is outdated? are there any user > level messages? The cache refers to files using their "build-id", which is different for every build. If you modify a source file and rebuild, the resulting binary will have a new build-id, for which there is no entry in the cache. So the first time you load that new binary, a new index will be recomputed. >> - The saved index file is exactly the same as the output of the "save >> gdb-index" command. It is therefore the exact same content that >> would >> be found in the .gdb_index or .debug_names section. We just leave >> it >> as a standalone file instead of merging it in the binary. > > This should be mentioned in the documentation, IMO. Ok, I thought this was an internal GDB implementation detail (which could be subject to change), but I can add it if you think it is important. >> - The code was made to follow the XDG specification: if the >> XDG_CACHE_HOME environment variable, it is used, otherwise it falls >> back to ~/.cache/gdb. It is possible to change it using "set >> index-cache directory". On other OSes than GNU/Linux, ~/.cache may >> not be the best place to put such data. On macOS it should probably >> default to ~/Library/Caches/... On Windows, %LocalAppData%/... I >> don't intend to do this part, but further patches are welcome. > > IMO, we should only use LocalAppData on Windows if we already do that > for .gdbinit etc. (And the platform standards on Windows actually > frown on putting files into that directory; you are supposed to put > them in an application-specific subdirectory.) Thanks for the info. I don't really know how things work on Windows... >> - I think that we need to be careful that multiple instances of GDB >> don't interfere with each other (not far fetched at all if you run >> GDB >> in some automated script) and the cache is always coherent (either >> the >> file is not found, or it is found and entirely valid). Writing the >> file directly to its final location seems like a recipe for failure. >> One GDB could read a file in the index while it is being written by >> another GDB. To mitigate this, I made write_psymtabs_to_index write >> to temporary files and rename them once it's done. Two GDB >> instances >> writing the index for the same file should not step on each other's >> toes (the last file to be renamed will stay). A GDB looking up a >> file >> will only see a complete file or no file. Also, if GDB crashes >> while >> generating the index file, it will leave a work-in-progress file, >> but >> it won't be picked up by other instances looking up in the cache. > > Maybe we should consider some kind of interlocking, like Emacs does. That may be needed when we implement some more advanced things, like auto-deletion of entries when the cache has grown past a certain size. >> + while (1) >> + { >> + /* Find the beginning of the next component. */ >> + while (*component_start == '/') >> + component_start++; >> + >> + /* Are we done? */ >> + if (*component_start == '\0') >> + return; >> + >> + /* Find the slash or null-terminator after this component. */ >> + component_end = component_start; >> + while (*component_end != '/' && *component_end != '\0') >> + component_end++; > > I believe literal uses of '/' are non-portable; you should use > IS_DIR_SEPARATOR (defined in filenames.h) instead. You also cannot > portably assume an absolute file name always starts with a slash. Ok. >> + /* If we get EEXIST and the existing path is a directory, then >> we're >> + happy. If it exists, but it's a regular file and this is >> not the last >> + component, we'll fail at the next component. If this is the >> last >> + component, the caller will fail with ENOTDIR when trying to >> + open/create a file under that path. */ >> + if (mkdir (start, 0700) != 0) > > This will not work on Windows. Which part of it? >> + if (errno != EEXIST) >> + return; > > I think on Windows the value of errno is different. Ok. >> + std::string filename = make_index_filename (build_id, >> INDEX4_SUFFIX); >> + >> + TRY >> + { >> + if (debug_index_cache) >> + printf_unfiltered ("index cache: trying to read %s\n", >> + filename.c_str ()); >> + >> + /* Try to map that file. */ >> + index_cache_resource_mmap *mmap_resource >> + = new index_cache_resource_mmap (filename.c_str ()); >> + >> + /* Yay, it worked! Hand the resource to the caller. */ >> + resource->reset (mmap_resource); >> + >> + return gdb::array_view >> + ((const gdb_byte *) mmap_resource->mapping.get (), >> + mmap_resource->mapping.size ()); >> + } >> + CATCH (except, RETURN_MASK_ERROR) >> + { >> + if (debug_index_cache) >> + printf_unfiltered ("index cache: couldn't read %s: %s\n", >> + filename.c_str (), except.message); >> + } >> + END_CATCH > > Does this mean this feature will only work on systems with mmap? Indeed. There is an equivalent API for Windows though I think: https://docs.microsoft.com/en-us/windows/desktop/Memory/file-mapping I can't really do the Windows bits, because I just don't have the competence to write software for Windows. Every time I tried to build GDB for Windows, I failed miserably (do we have a guide for that?). So my thought (including for the mkdir_recursive function) was that somebody who used Windows could later implement the missing parts. Also, I guess that somebody debugging native executables on Windows wouldn't have a use for the cache, because indices only work for DWARF debug info. Simon