[PATCH] backends: Handle new RISC-V specific definitions
Handle PT_RISCV_ATTRIBUTES, SHT_RISCV_ATTRIBUTES, DT_RISCV_VARIANT_CC.
Signed-off-by: Andreas Schwab
---
backends/riscv_init.c | 4
backends/riscv_symbol.c | 45 +
2 files changed, 49 insertions(+)
diff --git a/backends/riscv_init.c b/backends/riscv_init.c
index 141e0821..f2d46082 100644
--- a/backends/riscv_init.c
+++ b/backends/riscv_init.c
@@ -65,6 +65,10 @@ riscv_init (Elf *elf,
HOOK (eh, check_special_symbol);
HOOK (eh, machine_flag_check);
HOOK (eh, set_initial_registers_tid);
+ HOOK (eh, segment_type_name);
+ HOOK (eh, section_type_name);
+ HOOK (eh, dynamic_tag_name);
+ HOOK (eh, dynamic_tag_check);
if (eh->class == ELFCLASS64)
eh->core_note = riscv64_core_note;
else
diff --git a/backends/riscv_symbol.c b/backends/riscv_symbol.c
index c34b7702..c149b8ba 100644
--- a/backends/riscv_symbol.c
+++ b/backends/riscv_symbol.c
@@ -119,3 +119,48 @@ riscv_check_special_symbol (Elf *elf, const GElf_Sym *sym,
return false;
}
+
+const char *
+riscv_segment_type_name (int segment, char *buf __attribute__ ((unused)),
+size_t len __attribute__ ((unused)))
+{
+ switch (segment)
+{
+case PT_RISCV_ATTRIBUTES:
+ return "RISCV_ATTRIBUTES";
+}
+ return NULL;
+}
+
+/* Return symbolic representation of section type. */
+const char *
+riscv_section_type_name (int type,
+char *buf __attribute__ ((unused)),
+size_t len __attribute__ ((unused)))
+{
+ switch (type)
+{
+case SHT_RISCV_ATTRIBUTES:
+ return "RISCV_ATTRIBUTES";
+}
+
+ return NULL;
+}
+
+const char *
+riscv_dynamic_tag_name (int64_t tag, char *buf __attribute__ ((unused)),
+ size_t len __attribute__ ((unused)))
+{
+ switch (tag)
+{
+case DT_RISCV_VARIANT_CC:
+ return "RISCV_VARIANT_CC";
+}
+ return NULL;
+}
+
+bool
+riscv_dynamic_tag_check (int64_t tag)
+{
+ return tag == DT_RISCV_VARIANT_CC;
+}
--
2.37.1
--
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
[PATCH] elflint: Allow zero p_memsz for PT_RISCV_ATTRIBUTES
The RISCV_ATTRIBUTES segment is not meant to be loaded.
Signed-off-by: Andreas Schwab
---
src/ChangeLog | 5 +
src/elflint.c | 5 -
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/ChangeLog b/src/ChangeLog
index 42ce6640..fbcef29e 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,8 @@
+2022-08-09 Andreas Schwab
+
+ * elflint.c (check_program_header): Don't complain about p_filesz
+ > p_memsz if p_memsz is zero and p_type is PT_RISCV_ATTRIBUTES.
+
2022-08-01 Mark Wielaard
* readelf.c (handle_dynamic): Pass start of buffer to memrchr.
diff --git a/src/elflint.c b/src/elflint.c
index d919936f..b0e5415e 100644
--- a/src/elflint.c
+++ b/src/elflint.c
@@ -4731,7 +4731,10 @@ section [%2zu] '%s' must not be executable\n"),
}
if (phdr->p_filesz > phdr->p_memsz
- && (phdr->p_memsz != 0 || phdr->p_type != PT_NOTE))
+ && (phdr->p_memsz != 0
+ || (phdr->p_type != PT_NOTE
+ && !(ehdr->e_machine == EM_RISCV
+ && phdr->p_type == PT_RISCV_ATTRIBUTES
ERROR (_("\
program header entry %d: file size greater than memory size\n"),
cnt);
--
2.37.1
--
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Re: Making DT_HASH optional?
Hi, CCing the elfutils-devel list where this issue (of missing DT_SYMTAB_COUNT or DT_SYMTABSZ) does come up occasionally. Which are needed if you only have the dynamic segment, and not the section headers, to enumerate all symbols. On Mon, Aug 08, 2022 at 05:29:19PM -0700, Cary Coutant wrote: > > We recently had a case where dropping DT_HASH from the generic > > glibc binaries broke an ELF consumer. > > > > For the "real world" details see: > > https://sourceware.org/pipermail/libc-alpha/2022-August/141302.html > > > > I was surprised to see that DT_HASH was "mandatory" in the existing > > published standard. > > > > Why is it mandatory and not optional? > > > > Should we and could we make it optional? > > This is an odd situation. > > DT_HASH is mandatory because the "nchain" field in the hash table is > the only way to know the number of symbols in DT_SYMTAB (unless you > still have a section table and look for the .dynsym section, which is > not something we want to require). But when the psABI has effectively > replaced DT_HASH with something newer, it makes sense for that > requirement to transfer to the newer table. I suppose the gABI could > say something like "mandatory unless the psABI provides for a > replacement in some form," but that doesn't feel right to me. In this > case, I think it's simply up to the psABI to override the gABI and say > that DT_HASH is optional if DT_GNU_HASH is present. Unfortunately, > that means that generic tools that know nothing of ELFOSABI_GNU or > ELFOSABI_LINUX would be unable to process DT_SYMTAB if they can't find > DT_HASH. > > I'd probably have preferred to have a mandatory DT_SYMTAB_COUNT or > DT_SYMTABSZ entry in the dynamic table, which would have enabled us to > make the hash table (in any form) optional. But the DT_HASH > requirement has been with us since the beginning of time. Do you know the reason for not having a DT_SYMTAB_COUNT or DT_SYMTABSZ? It is not intuitive that one can find the number of entries through the hash table. I don't think DT_GNU_HASH can simply be made optional and replace DT_HASH without adding something like DT_SYMTAB_COUNT or DT_SYMTABSZ because the gnu hashtable doesn't come with a simple symbol count. To find the symbol count you have to go through the whole gnu hash table to count the number of entries (which is a non-trivial amount of work). Cheers, Mark
Re: [PATCH] backends: Handle new RISC-V specific definitions
On Tue, Aug 09, 2022 at 09:54:28AM +0200, Andreas Schwab wrote: > Handle PT_RISCV_ATTRIBUTES, SHT_RISCV_ATTRIBUTES, DT_RISCV_VARIANT_CC. Thanks, looks good. Added a ChangeLog entry and pushed. Cheers, Mark
Re: [PATCH] elflint: Allow zero p_memsz for PT_RISCV_ATTRIBUTES
Hi Andreas, On Tue, Aug 09, 2022 at 10:29:53AM +0200, Andreas Schwab wrote: > The RISCV_ATTRIBUTES segment is not meant to be loaded. That makes sense. Pushed. Thanks, Mark
☝ Buildbot (GNU Toolchain): elfutils - worker not available (master)
A retry build has been detected on builder elfutils-debian-amd64 while building elfutils. Full details are available at: https://builder.sourceware.org/buildbot/#builders/52/builds/59 Build state: worker not available Revision: (unknown) Worker: bb2-2 Build Reason: (unknown) Blamelist: Andreas Schwab Steps: - 0: worker_preparation ( exception ) Logs: - err.text: https://builder.sourceware.org/buildbot/#builders/52/builds/59/steps/0/logs/err_text - err.html: https://builder.sourceware.org/buildbot/#builders/52/builds/59/steps/0/logs/err_html A retry build has been detected on builder elfutils-debian-amd64 while building elfutils. Full details are available at: https://builder.sourceware.org/buildbot/#builders/52/builds/60 Build state: worker not available Revision: (unknown) Worker: bb2-2 Build Reason: (unknown) Blamelist: Andreas Schwab Steps: - 0: worker_preparation ( exception ) Logs: - err.text: https://builder.sourceware.org/buildbot/#builders/52/builds/60/steps/0/logs/err_text - err.html: https://builder.sourceware.org/buildbot/#builders/52/builds/60/steps/0/logs/err_html A retry build has been detected on builder elfutils-debian-amd64 while building elfutils. Full details are available at: https://builder.sourceware.org/buildbot/#builders/52/builds/61 Build state: worker not available Revision: (unknown) Worker: bb2-2 Build Reason: (unknown) Blamelist: Andreas Schwab Steps: - 0: worker_preparation ( exception ) Logs: - err.text: https://builder.sourceware.org/buildbot/#builders/52/builds/61/steps/0/logs/err_text - err.html: https://builder.sourceware.org/buildbot/#builders/52/builds/61/steps/0/logs/err_html
cannot skip augment string handling
He dwarf_next_cfi function has some clever code which skips over the
processing of the augmentation string content if the first character is 'z'
(for sized augmentation). This would be OK if it wouldn't be for the fact
that the augment processing loop produces additional information, namely,
it fills in the fde_augmentation_data_size fields. That information isn't
available elsewhere.
In addition, the loop over the augment string is incorrect because the
interpretation of the P, L, and R entries depends on 'z' being present. in
the absence of 'z', when the loop would be executed in the current version,
the interpretation of those entries is not the same.
In the patch below I've removed the shortcut and fixed the handling of the
P, L, and R entries. I've also added an additional test checking that the
entries of the augmentation string don't guide the code to consume more
data then is indicated in the 'z' data.
libdw/ChangeLog
2022-08-09 Ulrich Drepper
* dwarf_next_cfi.c (dwarf_next_cfi): Don't skip processing the
augmentation string. Be more stringent what to accept.
diff --git a/libdw/dwarf_next_cfi.c b/libdw/dwarf_next_cfi.c
index fa28d99b..23b16885 100644
--- a/libdw/dwarf_next_cfi.c
+++ b/libdw/dwarf_next_cfi.c
@@ -193,50 +193,71 @@ dwarf_next_cfi (const unsigned char e_ident[],
else /* DWARF 2 */
entry->cie.return_address_register = *bytes++;
- /* If we have sized augmentation data,
- we don't need to grok it all. */
entry->cie.fde_augmentation_data_size = 0;
+ entry->cie.augmentation_data = bytes;
bool sized_augmentation = *ap == 'z';
if (sized_augmentation)
{
+ ++ap;
if (bytes >= limit)
goto invalid;
get_uleb128 (entry->cie.augmentation_data_size, bytes, limit);
if ((Dwarf_Word) (limit - bytes) < entry->cie.augmentation_data_size)
goto invalid;
entry->cie.augmentation_data = bytes;
- bytes += entry->cie.augmentation_data_size;
}
- else
- {
- entry->cie.augmentation_data = bytes;
- for (; *ap != '\0'; ++ap)
+ for (; *ap != '\0'; ++ap)
+ {
+ uint8_t encoding;
+ switch (*ap)
{
- uint8_t encoding;
- switch (*ap)
+ case 'L':
+ if (sized_augmentation)
{
- case 'L': /* Skip LSDA pointer encoding byte. */
- case 'R': /* Skip FDE address encoding byte. */
+ /* Skip LSDA pointer encoding byte. */
encoding = *bytes++;
entry->cie.fde_augmentation_data_size
+= encoded_value_size (data, e_ident, encoding, NULL);
continue;
- case 'P': /* Skip encoded personality routine pointer. */
+ }
+ break;
+ case 'R':
+ if (sized_augmentation)
+ {
+ /* Skip FDE address encoding byte. */
encoding = *bytes++;
- bytes += encoded_value_size (data, e_ident, encoding, bytes);
continue;
- case 'S': /* Skip signal-frame flag. */
+ }
+ break;
+ case 'P':
+ if (sized_augmentation)
+ {
+ /* Skip encoded personality routine pointer. */
+ encoding = *bytes++;
+ bytes += encoded_value_size (data, e_ident, encoding, bytes);
continue;
- default:
- /* Unknown augmentation string. initial_instructions might
- actually start with some augmentation data. */
- break;
}
break;
+ case 'S':
+ if (sized_augmentation)
+ /* Skip signal-frame flag. */
+ continue;
+ break;
+ default:
+ /* Unknown augmentation string. initial_instructions might
+ actually start with some augmentation data. */
+ break;
}
- entry->cie.augmentation_data_size
- = bytes - entry->cie.augmentation_data;
+ break;
+ }
+ if (! sized_augmentation)
+ entry->cie.augmentation_data_size = bytes - entry->cie.augmentation_data;
+ else
+ {
+ if (bytes > entry->cie.augmentation_data + entry->cie.augmentation_data_size)
+ goto invalid;
+ bytes = entry->cie.augmentation_data + entry->cie.augmentation_data_size;
}
entry->cie.initial_instructions = bytes;
Re: debuginfod Credential Helper RFC
On Mon, Aug 8, 2022 at 1:41 PM Frank Ch. Eigler wrote:
> So-so ... if the file contents are modified, but the environment
> variable that points to the file is fixed, then one may get into parse
> race conditions as different debuginfod client objects in the process
> may be active at the same time.
>
Ah, that's a good point. To support dynamic updates you'd need to
completely reload config for each query, which is prohibitive, and you may
get inconsistencies in behavior. So that just leaves file permissioning as
a use case.
>
> > [...] You could also do this more granularly:
> > DEBUGINFOD_HEADERS_FILES would work for us, and other lists could be
> > created for other dynamically controllable aspects of the system.
> > [...]
>
> I see some value in doing this sort of thing more broadly,
> hypothetically, but it's vague/speculative enough that I'd be just as
> glad to limit the concept to the present case ("also add all headers
> in given file"). So how about a $DEBUGINFOD_HEADERS and perhaps
> $DEBUGINFOD_HEADERS_FILE env vars for now?
>
Sounds good to me. If permissions are the only benefit to ..._FILE
environment variables, then headers are the only bit of config that it
makes sense to access control, so it makes sense as a special case.
--
Daniel Thornburgh | dth...@google.com
One shot mode for debuginfod server
Hi,
Here is a patch for a new one-shot mode for the debuginfod server. In this
mode the first scanning pass of the server will also output the found
executables' paths and buildids to the given file descriptor. This can (and
soon will be used in systemtap) as an easy way to scan archive files and
quickly/easily extract information concerning the executables they contain.
The patch passes all of the tests across the try-buildbots (branch
users/rgoldber/try-one-shot)
Thanks,
Ryan (rgoldber)
From a4e5d1bbd39d2448e5ae208a114e3a68d9f2c28a Mon Sep 17 00:00:00 2001
From: Ryan Goldberg
Date: Mon, 8 Aug 2022 13:30:08 -0400
Subject: [PATCH] One-shot mode for server
When in one-shot mode, for the first
scan all of the found executables' paths
and buildids will be written to the provided
file descriptor in the given format.
Signed-off-by: Ryan Goldberg
---
configure.ac | 5 ++
debuginfod/ChangeLog | 9 +++
debuginfod/Makefile.am | 2 +-
debuginfod/debuginfod.cxx| 52 ++-
debuginfod/debuginfod.h.in | 9 +++
doc/ChangeLog| 4 ++
doc/debuginfod.8 | 16 +
tests/ChangeLog | 7 ++-
tests/Makefile.am| 5 +-
tests/run-debuginfod-one-shot.sh | 105 +++
10 files changed, 209 insertions(+), 5 deletions(-)
create mode 100755 tests/run-debuginfod-one-shot.sh
diff --git a/configure.ac b/configure.ac
index 03b67a9d..aadff4ca 100644
--- a/configure.ac
+++ b/configure.ac
@@ -600,6 +600,11 @@ case "$ac_cv_search__obstack_free" in
esac
AC_SUBST([obstack_LIBS])
+AC_CHECK_LIB(json-c, json_tokener_parse, [
+ AC_DEFINE([ENABLE_ONE_SHOT], [1], [Define if the one-shot feature is enabled])
+ AC_SUBST(jsonc_LIBS, '-ljson-c')
+])
+
dnl The directories with content.
dnl Documentation.
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index a8b5d7f4..b6fac996 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,12 @@
+2022-08-09 Ryan Goldberg
+
+ * debuginfod.cxx (archive_classify): New global (one_shot_mtx).
+ (archive_classify, thread_main_fts_source_paths) In one-shot mode
+ for the first scan, output found buildids/paths to given file descriptor.
+ (parse_opt): Add "--one-shot" flag.
+ * debuginfod.h.in: Added in debuginfod_oneshot_header struct,
+ debuginfod_oneshot_header_magic
+
2022-08-02 Josef Cejka
* debuginfod.cxx (groom): Don't evaluate regex unless needed.
diff --git a/debuginfod/Makefile.am b/debuginfod/Makefile.am
index 435cb8a6..03cdeed0 100644
--- a/debuginfod/Makefile.am
+++ b/debuginfod/Makefile.am
@@ -70,7 +70,7 @@ bin_PROGRAMS += debuginfod-find
endif
debuginfod_SOURCES = debuginfod.cxx
-debuginfod_LDADD = $(libdw) $(libelf) $(libeu) $(libdebuginfod) $(argp_LDADD) $(fts_LIBS) $(libmicrohttpd_LIBS) $(sqlite3_LIBS) $(libarchive_LIBS) -lpthread -ldl
+debuginfod_LDADD = $(libdw) $(libelf) $(libeu) $(libdebuginfod) $(argp_LDADD) $(fts_LIBS) $(libmicrohttpd_LIBS) $(sqlite3_LIBS) $(libarchive_LIBS) $(jsonc_LIBS) -lpthread -ldl
debuginfod_find_SOURCES = debuginfod-find.c
debuginfod_find_LDADD = $(libdw) $(libelf) $(libeu) $(libdebuginfod) $(argp_LDADD) $(fts_LIBS)
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index a089d0bd..04c5f635 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -54,10 +54,12 @@ extern "C" {
#include
#include
#include
-#include
#include
#include
+#ifdef ENABLE_ONE_SHOT
+#include
+#endif
/* If fts.h is included before config.h, its indirect inclusions may not
give us the right LFS aliases of these functions, so map them manually. */
@@ -387,6 +389,8 @@ static const struct argp_option options[] =
{ "passive", ARGP_KEY_PASSIVE, NULL, 0, "Do not scan or groom, read-only database.", 0 },
#define ARGP_KEY_DISABLE_SOURCE_SCAN 0x1009
{ "disable-source-scan", ARGP_KEY_DISABLE_SOURCE_SCAN, NULL, 0, "Do not scan dwarf source info.", 0 },
+#define ARGP_KEY_ONE_SHOT 0x100a
+ { "one-shot", ARGP_KEY_ONE_SHOT, "NUM", 0, "For the first scan, output found buildids/paths to given file descriptor", 0 },
{ NULL, 0, NULL, 0, NULL, 0 },
};
@@ -439,6 +443,8 @@ static unsigned forwarded_ttl_limit = 8;
static bool scan_source_info = true;
static string tmpdir;
static bool passive_p = false;
+static int one_shot_fd = -1;
+static mutex one_shot_mtx;
static void set_metric(const string& key, double value);
// static void inc_metric(const string& key);
@@ -639,6 +645,16 @@ parse_opt (int key, char *arg,
// other conflicting options tricky to check
argp_failure(state, 1, EINVAL, "inconsistent options with passive mode");
break;
+case ARGP_KEY_ONE_SHOT:
+ one_shot_fd = atol(arg);
+ #ifdef ENABLE_ONE_SHOT
+if (one_shot_fd < 0)
+ argp_failure(state, 1, EINVAL, "one-shot fd");
+ #else
+argp_failure(state, 1, EINVAL, "Debuginfod in one-shot mode r
