Subject: Re: bug#78762: [PATCH] Fix segfault in profiler-cpu-log and profiler-memory-log

From: zach shaftel <[email-masked]>
Date: Thu, 12 Jun 2025 22:11:36 -0400

thank you for the feedback, a new patch is attached. let me know if i
missed anything.

- zach

>From b90a79d7b9845cf39a124648cc860bd7f48d858d Mon Sep 17 00:00:00 2001
From: Zach Shaftel <[email-masked]>
Date: Wed, 11 Jun 2025 15:37:31 -0400
Subject: [PATCH] Fix segfault in profiler-cpu-log and profiler-memory-log
 (bug#78763)

* src/profiler.c (export_log): Check if a log has been allocated first,
and return nil if it hasn't.
(Fprofiler_cpu_log, Fprofiler_memory_log): Doc fix.
* test/src/profiler-tests.el (profiler-tests-cpu-profiler)
(profiler-tests-memory-profiler): New tests.
---
 src/profiler.c             | 13 +++++++--
 test/src/profiler-tests.el | 58 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 2 deletions(-)
 create mode 100644 test/src/profiler-tests.el

diff --git a/src/profiler.c b/src/profiler.c
index 4c88f676fbd..f3a8db69f78 100644
--- a/src/profiler.c
+++ b/src/profiler.c
@@ -534,7 +534,11 @@ DEFUN ("profiler-cpu-log", Fprofiler_cpu_log, Sprofiler_cpu_log,
 The log is a hash-table mapping backtraces to counters which represent
 the amount of time spent at those points.  Every backtrace is a vector
 of functions, where the last few elements may be nil.
-Before returning, a new log is allocated for future samples.  */)
+
+If the profiler has not run since the last invocation of
+`profiler-cpu-log' (or was never run at all), return nil.  If the
+profiler is currently running, allocate a new log for future samples
+before returning.  */)
   (void)
 {
   /* Temporarily stop profiling to avoid it interfering with our data
@@ -556,6 +560,7 @@ DEFUN ("profiler-cpu-log", Fprofiler_cpu_log, Sprofiler_cpu_log,
 static Lisp_Object
 export_log (struct profiler_log *plog)
 {
+  if (!plog->log) return Qnil;
   log_t *log = plog->log;
   /* The returned hash table uses `equal' as key equivalence predicate
      which is more discriminating than the `function-equal' used by
@@ -639,7 +644,11 @@ DEFUN ("profiler-memory-log",
 The log is a hash-table mapping backtraces to counters which represent
 the amount of memory allocated at those points.  Every backtrace is a vector
 of functions, where the last few elements may be nil.
-Before returning, a new log is allocated for future samples.  */)
+
+If the profiler has not run since the last invocation of
+`profiler-memory-log' (or was never run at all), return nil.  If the
+profiler is currently running, allocate a new log for future samples
+before returning.  */)
   (void)
 {
   bool prof_mem = profiler_memory_running;
diff --git a/test/src/profiler-tests.el b/test/src/profiler-tests.el
new file mode 100644
index 00000000000..fe50de713e2
--- /dev/null
+++ b/test/src/profiler-tests.el
@@ -0,0 +1,58 @@
+;;; profiler-tests.el --- tests for src/profiler.c  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2025 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+
+(ert-deftest profiler-tests-memory-profiler ()
+  (let ((was-running (profiler-memory-running-p)))
+    ;; do this first in case the profiler was already running
+    (should (eq was-running (profiler-memory-stop)))
+    (profiler-memory-start)
+    (should-error (profiler-memory-start))
+    (should (profiler-memory-running-p))
+    (should (hash-table-p (profiler-memory-log)))
+    ;; `profiler-memory-log' shouldn't terminate profiling.
+    (should (profiler-memory-running-p))
+    (profiler-memory-stop)
+    (profiler-memory-log)               ;flush the log
+    (should-not (profiler-memory-log))
+    (when was-running (profiler-memory-start))))
+
+(defconst profiler-tests-cpu-sampling-interval 1000000)
+
+(ert-deftest profiler-tests-cpu-profiler ()
+  (skip-unless (fboundp 'profiler-cpu-start))
+  (let ((was-running (profiler-cpu-running-p)))
+    (should (eq was-running (profiler-cpu-stop)))
+    (profiler-cpu-start profiler-tests-cpu-sampling-interval)
+    (should-error (profiler-cpu-start profiler-tests-cpu-sampling-interval))
+    (should (hash-table-p (profiler-cpu-log)))
+    (should (profiler-cpu-running-p))
+    (profiler-cpu-stop)
+    (profiler-cpu-log)
+    (should-not (profiler-cpu-log))
+    (when was-running
+      (profiler-cpu-start (or (bound-and-true-p profiler-sampling-interval)
+                              profiler-tests-cpu-sampling-interval)))))
+
+;;; profiler-tests.el ends here
-- 
2.49.0