Re: Remove nested functions from readelf.c
On 01/02/2021 15:21, Mark Wielaard wrote: On Fri, 2021-01-08 at 09:16 +0100, Timm Bäder via Elfutils-devel wrote: here another round for src/readelf.c. I think they are simple, but I'm not happy with the advance_pc() commit. I'm open for suggestions here but I can't come up with something better right now. I'll keep looking in to that in the meantime. Yeah, the advance_pc function is really why you want nested functions in the first place IMHO. Passing around the captured state as 6 extra arguments seems to not really make the code much clearer. Especially because on its own it isn't immediately clear what the code is about or that it is to be used as part of the special opcode or DW_LNS_advance_pc opcode (where DW_LNS_const_add_pc acts like special opcode 255) handling. It might be helpful to spell out in a comment to the function which part of the DWARF4 6.2.5.1 Special Opcodes handling the advance_pc function is responsible for. But honestly in this case I think just keeping the nested function is the cleanest way to handle this. Cheers, Mark That's what I was worried about as well of course. But I think I can work on this a bit and solve it in a cleaner way, with some additional refactorings. I'll try to come up with some patches. - Timm -- Red Hat GmbH, http://www.de.redhat.com/, Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Michael O'Neill, Tom Savage, Eric Shander
[PATCH] readelf: Remove show_op_index variable
From: Timm Bäder
advance_pc() uses show_op_index to save whether the current op_index is
> 0 OR the new op_index is > 0.
The new op index is calculated via
new_op_index = (op_index + op_advance) % max_ops_per_instr;
since all of the variables involved are unsigned,
new_op_index >= op_index is always true.
So...
if op_index > 0, then new_op_index > 0
if op_index == 0, then new_op_index >= 0
and if the new_op_index is > 0, then the old one was as well.
In any case, we only need to check the new_op_index, since show_op_index
used to OR the two comparisons.
In other words:
op_index > 0 | new_op_index > 0 || show_op_index
true truetrue
false truetrue
true falsetrue xx
false falsefalse
... but since the third line (marked with xx) is not possible,
the table becomes:
op_index > 0 | new_op_index > 0 || show_op_index
true truetrue
false truetrue
false falsefalse
... and show_op_index is equal to (new_op_index > 0).
So, remove the show_op_index variable and simply replace it by comparing
the new op_index > 0.
---
src/ChangeLog | 5 +
src/readelf.c | 9 +++--
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/ChangeLog b/src/ChangeLog
index 1f53ca7b..9e30df31 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,8 @@
+2021-02-02 Timm Bäder
+
+ * readelf.c (print_debug_line_section): Remove unnecessary
+ show_op_index variable, replace with (op_index > 0).
+
2021-01-08 Timm Bäder
* readelf.c (print_cfa_program): Lift regname function to...
diff --git a/src/readelf.c b/src/readelf.c
index 9943c211..11692bb5 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -8752,14 +8752,11 @@ print_debug_line_section (Dwfl_Module *dwflmod, Ebl
*ebl, GElf_Ehdr *ehdr,
/* Apply the "operation advance" from a special opcode
or DW_LNS_advance_pc (as per DWARF4 6.2.5.1). */
unsigned int op_addr_advance;
- bool show_op_index;
inline void advance_pc (unsigned int op_advance)
{
op_addr_advance = minimum_instr_len * ((op_index + op_advance)
/ max_ops_per_instr);
address += op_addr_advance;
- show_op_index = (op_index > 0 ||
-(op_index + op_advance) % max_ops_per_instr > 0);
op_index = (op_index + op_advance) % max_ops_per_instr;
}
@@ -8803,7 +8800,7 @@ print_debug_line_section (Dwfl_Module *dwflmod, Ebl *ebl,
GElf_Ehdr *ehdr,
printf (_(" special opcode %u: address+%u = "),
opcode, op_addr_advance);
print_dwarf_addr (dwflmod, 0, address, address);
- if (show_op_index)
+ if (op_index > 0)
printf (_(", op_index = %u, line%+d = %zu\n"),
op_index, line_increment, line);
else
@@ -8921,7 +8918,7 @@ print_debug_line_section (Dwfl_Module *dwflmod, Ebl *ebl,
GElf_Ehdr *ehdr,
printf (_(" advance address by %u to "),
op_addr_advance);
print_dwarf_addr (dwflmod, 0, address, address);
- if (show_op_index)
+ if (op_index > 0)
printf (_(", op_index to %u"), op_index);
printf ("\n");
}
@@ -8982,7 +8979,7 @@ print_debug_line_section (Dwfl_Module *dwflmod, Ebl *ebl,
GElf_Ehdr *ehdr,
printf (_(" advance address by constant %u to "),
op_addr_advance);
print_dwarf_addr (dwflmod, 0, address, address);
- if (show_op_index)
+ if (op_index > 0)
printf (_(", op_index to %u"), op_index);
printf ("\n");
}
--
2.26.2
Re: [PATCH 3/4] ar: Pull should_truncate_fname() into file scope
On 29/01/2021 21:48, Mark Wielaard wrote: Hi Timm, On Fri, 2021-01-08 at 09:13 +0100, Timm Bäder via Elfutils-devel wrote: Get rid of a nested function this way. Skipping this one for now since I don't believe I understand this code. Could you explain what the code does what it does and why it works fine when moved this way? Right, I guess you mean because the old code used to assign a new value to name_max, but the new code doesn't do that? I fixed that locally now by taking a size_t* inout argument instead, so assigning to name_max should have the desired effect again. - Timm -- Red Hat GmbH, http://www.de.redhat.com/, Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Michael O'Neill, Tom Savage, Eric Shander
[Bug debuginfod/27323] New: concurrency during grooming/scanning
https://sourceware.org/bugzilla/show_bug.cgi?id=27323 Bug ID: 27323 Summary: concurrency during grooming/scanning Product: elfutils Version: unspecified Status: NEW Severity: normal Priority: P2 Component: debuginfod Assignee: unassigned at sourceware dot org Reporter: fche at redhat dot com CC: elfutils-devel at sourceware dot org Target Milestone: --- Further to bug #26775, which made grooming interruptible, we need more. Grooming involves some long-running operations, like the database-stats count(*) queries, which can take MINUTES of run time on a large server. sqlite's single-thread-per-connection model means that during those times, web queries cannot be served, and just pile up. We need to add a little more concurrency to debuginfod, at the sqlite connection level. -- You are receiving this mail because: You are on the CC list for the bug.
PATCH PR27323 debuginfod concurrency++
Hi -
Some patches for the 0.183 release. This and PR27092 (low memory)
will be important, maybe PR27277 coming too.
commit d1608c045c46d2ab3d42ef668474e365c4b54e84 (HEAD -> master)
Author: Frank Ch. Eigler
Date: Tue Feb 2 16:49:19 2021 -0500
PR27323 debuginfod: improve query concurrency with grooming
Start using a second sqlite3 database connection for webapi query
servicing. This allows much better concurrency when long-running
grooming operations are in progress.
No testsuite impact. Grooming times are too short to try to hit with
concurrent requests. OTOH the existing tests did show some
interesting regressions that needed fixing, like needing not to
dual-wield db and dbq when doing rpm-dwz-related lookups from during
scanning, and the way in which corrupted databases are reported.
These needed some automated invocations of gdb on the running
debuginfod binaries that just failed their testing, for in-situ
debugging.
Hand-tested for function on a huge 20GB index file. Allowed webapi
queries to be run throughout random points of the grooming process,
including especially the long count(*) report loops before & after.
Signed-off-by: Frank Ch. Eigler
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 43a3b9a37932..2872d667fc37 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,17 @@
+2021-02-02 Frank Ch. Eigler
+
+ PR27323
+ * debuginfod.cxx (dbq): New read-only database connection for queries
+ only.
+ (signal_handler): Interrupt it.
+ (main): Open / close it.
+ (handle_buildid): Use it for webapi queries only.
+ (database_stats_report): Make more interruptible. Report sqlite3
+ operation times to the prometheus metrics.
+ (groom): Make more interruptible.
+ (thread_main_fts_source_paths, thread_main_groom): Ensure
+ state/progress metrics are fresh even in case of exceptions.
+
2020-12-20 Dmitry V. Levin
* .gitignore: New file.
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 9a6971868277..c9c0dc9bb819 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -1,5 +1,5 @@
/* Debuginfo-over-http server.
- Copyright (C) 2019-2020 Red Hat, Inc.
+ Copyright (C) 2019-2021 Red Hat, Inc.
This file is part of elfutils.
This file is free software; you can redistribute it and/or modify
@@ -385,7 +385,8 @@ static struct argp argp =
static string db_path;
-static sqlite3 *db; // single connection, serialized across all our threads!
+static sqlite3 *db; // single connection, serialized across all our threads!
+static sqlite3 *dbq; // webapi query-servicing readonly connection, serialized
ditto!
static unsigned verbose;
static volatile sig_atomic_t interrupted = 0;
static volatile sig_atomic_t forced_rescan_count = 0;
@@ -1569,11 +1570,15 @@ handle_buildid (MHD_Connection* conn,
obatched(clog) << "searching for buildid=" << buildid << " artifacttype="
<< artifacttype
<< " suffix=" << suffix << endl;
+ // If invoked from the scanner threads, use the scanners' read-write
+ // connection. Otherwise use the web query threads' read-only connection.
+ sqlite3 *thisdb = (conn == 0) ? db : dbq;
+
sqlite_ps *pp = 0;
if (atype_code == "D")
{
- pp = new sqlite_ps (db, "mhd-query-d",
+ pp = new sqlite_ps (thisdb, "mhd-query-d",
"select mtime, sourcetype, source0, source1 from "
BUILDIDS "_query_d where buildid = ? "
"order by mtime desc");
pp->reset();
@@ -1581,7 +1586,7 @@ handle_buildid (MHD_Connection* conn,
}
else if (atype_code == "E")
{
- pp = new sqlite_ps (db, "mhd-query-e",
+ pp = new sqlite_ps (thisdb, "mhd-query-e",
"select mtime, sourcetype, source0, source1 from "
BUILDIDS "_query_e where buildid = ? "
"order by mtime desc");
pp->reset();
@@ -1593,7 +1598,7 @@ handle_buildid (MHD_Connection* conn,
// Incoming source queries may come in with either dwarf-level OR
canonicalized paths.
// We let the query pass with either one.
- pp = new sqlite_ps (db, "mhd-query-s",
+ pp = new sqlite_ps (thisdb, "mhd-query-s",
"select mtime, sourcetype, source0, source1 from "
BUILDIDS "_query_s where buildid = ? and artifactsrc in (?,?) "
"order by sharedprefix(source0,source0ref) desc,
mtime desc");
pp->reset();
@@ -1629,6 +1634,7 @@ handle_buildid (MHD_Connection* conn,
if (r)
return r;
}
+ pp->reset();
// We couldn't find it in the database. Last ditch effort
// is to defer to other debuginfo servers.
@@ -2971,19 +2977,21 @@ thread_main_fts_source_paths (void* arg)
rescan_now = true;
}
if (rescan_now)
-try
-
