Re: [PATCH] debuginfod: Support queries for ELF/DWARF sections
Hi, On Mon, 2022-10-24 at 14:38 -0400, Frank Ch. Eigler via Elfutils-devel wrote: > - not sure I understand why the code worries about dots in or not in > section names. Why not just pass them verbatim throughout the code > base, and not worry about whether or not there's a dot? Does the > ELF standard even require a dot? I agree that just passing them through as is might be better. The ELF standard doesn't say much about section names, just: Section names with a dot (.) prefix are reserved for the system, although applications may use these sections if their existing meanings are satisfactory. Applications may use names without the prefix to avoid conflicts with system sections. Is/should the section name be URL-encoded? I would drop the maybe_debuginfo_section heuristics. There are some sections like .strtab/.symtab that are probably in the debug file, but might be in the executable. I would assume that a named section can normally be found in the debugfile and only use the executable as fallback. So see if you can find the .debug file, if you can, then look for the section by name. If it isn't SHT_NOBITS you found it. If it is SHT_NOBITS the section should be in the exe. If the section cannot be found by name (in the .debug file) you can stop searching, it also won't be in the exe. If you cannot find the .debug file, or the section was in the .debug file, but had type SHT_NOBITS then search for the exe file and the named section in there. Finally, if the section comes from a file in the cache or if we have to download it in full anyway, then extracting the section into its own file seems slightly wasteful. It would be great if we could just report back "here is the full exe/debug file which does contain the requested section name". But that might make the interface a little ugly. int debuginfod_find_section (debuginfod_client *client, const unsigned char *build_id, int build_id_len, const char *section, char **path, bool *file_is_elf) Maybe that is over-designed to avoid a little bit of disk waste? Cheers, Mark
Re: [PATCH] debuginfod: Support queries for ELF/DWARF sections
Hi, On Mon, 2022-10-24 at 14:38 -0400, Frank Ch. Eigler via Elfutils-devel wrote: > - use of write(2) to put files onto disk is not quite right; write(2) > can > be partial, so you need a loop (or a macro wrapping a loop) Since debuginfod-client.c already includes system.h it can use: static inline ssize_t write_retry (int fd, const void *buf, size_t len) Which takes care of partial and/or interrupted write calls. Cheers, Mark
Re: [PATCH] debuginfod: Support queries for ELF/DWARF sections
Hi - > Is/should the section name be URL-encoded? Yes! > I would drop the maybe_debuginfo_section heuristics. There are some > sections like .strtab/.symtab that are probably in the debug file, but > might be in the executable. I would assume that a named section can > normally be found in the debugfile and only use the executable as > fallback. That heuristic would work fine for the case of .gdb_index, that motivated this whole piece of work. Sure. > Finally, if the section comes from a file in the cache or if we have to > download it in full anyway, then extracting the section into its own > file seems slightly wasteful. It would be great if we could just report > back "here is the full exe/debug file which does contain the requested > section name". [...] It'd be fine to pass back the extracted section content anyway, even if the full elf and/or dwarf file is already there. Consider federated debuginfod servers. Intermediate servers may be willing/able to do this extraction on behalf of clients who really only want the section in question. And if they cache the result, as in amerey's draft code, then this will also help accelerate other future clients. That's just the usage scenario (gdb acceleration). > int > debuginfod_find_section (debuginfod_client *client, > const unsigned char *build_id, > int build_id_len, > const char *section, char **path, > bool *file_is_elf) > > Maybe that is over-designed to avoid a little bit of disk waste? (Then the client code would have to learn elfutils API internals in order to extract the section it was actually interested in.) - FChE
Re: [PATCH] debuginfod: Support queries for ELF/DWARF sections
Hi Frank, On Mon, Oct 24, 2022 at 2:38 PM Frank Ch. Eigler wrote: > - use of write(2) to put files onto disk is not quite right; write(2) can > be partial, so you need a loop (or a macro wrapping a loop) Fixed. > - not sure I understand why the code worries about dots in or not in > section names. Why not just pass them verbatim throughout the code > base, and not worry about whether or not there's a dot? Does the > ELF standard even require a dot? Fair point. The motivation for providing a general section query API as opposed to an API for specific indices was to be flexible and accommodate a wide range of use cases. Insisting on the dot is contrary to this goal. > - not sure whether/why the I queries require a new _query_i view, as > opposed to running the _query_d & _query_e views union'd together. > I see an ORDER BY that's different here but not sure why bother; > if anything, the server could prefer one or the other type, based > on the same section-name heuristic as the client The idea here was to prevent cases where a section would be extracted from the executable on one server and from the debuginfo on another server. Granted this is only relevant when the file mtimes differ between servers and for sections that differ between debuginfo and executable. This situation seems unlikely and these particular sections aren't of much interest (ex. .shstrtab). I will remove this view and just use the existing ones. > - don't really see a need for the X-DEBUGINFOD-SECTION response > header, which simply echoes back the very same parameter the client > just requested; the other X-DEBUGINFOD-* headers are novel metadata > > - re. verbose logging in the section vs non-section case, suggest > just keeping the code simple (even if it makes the logs more verbose), > i.e., not duplicating if (...) clog << STUFF else clog << STUFF; > no biggie tho > > - the webapi docs in debuginfod.8 should document the new query type Fixed. Aaron
Re: [PATCH] debuginfod: Support queries for ELF/DWARF sections
Hi Mark, On Wed, Oct 26, 2022 at 11:06 AM Mark Wielaard wrote: > On Mon, 2022-10-24 at 14:38 -0400, Frank Ch. Eigler via Elfutils-devel > wrote: > > - not sure I understand why the code worries about dots in or not in > > section names. Why not just pass them verbatim throughout the code > > base, and not worry about whether or not there's a dot? Does the > > ELF standard even require a dot? > > I agree that just passing them through as is might be better. The ELF > standard doesn't say much about section names, just: > >Section names with a dot (.) prefix are reserved for the system, >although applications may use these sections if their existing >meanings are satisfactory. Applications may use names without the >prefix to avoid conflicts with system sections. Agreed, will fix. > I would drop the maybe_debuginfo_section heuristics. There are some > sections like .strtab/.symtab that are probably in the debug file, but > might be in the executable. I would assume that a named section can > normally be found in the debugfile and only use the executable as > fallback. > > So see if you can find the .debug file, if you can, then look for the > section by name. If it isn't SHT_NOBITS you found it. If it is > SHT_NOBITS the section should be in the exe. If the section cannot be > found by name (in the .debug file) you can stop searching, it also > won't be in the exe. If you cannot find the .debug file, or the section > was in the .debug file, but had type SHT_NOBITS then search for the exe > file and the named section in there. I like this heuristic. It's simpler and we don't have to update anything if/when a new section becomes common. > Finally, if the section comes from a file in the cache or if we have to > download it in full anyway, then extracting the section into its own > file seems slightly wasteful. It would be great if we could just report > back "here is the full exe/debug file which does contain the requested > section name". But that might make the interface a little ugly. > > int > debuginfod_find_section (debuginfod_client *client, > const unsigned char *build_id, > int build_id_len, > const char *section, char **path, > bool *file_is_elf) > > Maybe that is over-designed to avoid a little bit of disk waste? We'd save a bit of disk space but complicate the API and often cause client tools to have to do more work to get at the section contents. In the case of .gdb_index, gdb already knows how to read the index from a separate file. Of course it can read the section from an ELF file too but I suspect there might need to be some changes to teach it how to handle "unusual" ELF files that only contain a single section. > Since debuginfod-client.c already includes system.h it can use: > > static inline ssize_t > write_retry (int fd, const void *buf, size_t len) > > Which takes care of partial and/or interrupted write calls. Thanks that's exactly what I'm looking for. Aaron
