Skip to content

Commit 60af820

Browse files
committed
Fix data-race and disjoint-member contract comments
Three contract/implementation contradictions found in a second audit pass: - on(DiagsTagType): @note said "reads atomic state"; _enabled is a plain int — corrected to describe the known data race. - DiagsConfigState::enabled read overload: @note said "Thread-safe for reads"; same plain-int issue — corrected to "data race; known legacy". - should_roll_outputlog(): @note listed reseat_diagslog() as a concurrent concern; the two methods operate on disjoint members (diags_log vs stdout_log/stderr_log) — removed the incorrect cross-reference.
1 parent 65accbe commit 60af820

1 file changed

Lines changed: 35 additions & 26 deletions

File tree

include/tscore/DiagsTypes.h

Lines changed: 35 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,10 @@ class DiagsConfigState
8686
///
8787
/// @param dtt DiagsTagType_Debug or DiagsTagType_Action.
8888
/// @return 0 if disabled, 1 if globally enabled, 2 if IP-override enabled.
89-
/// @note Intended as a fast-path check. Thread-safe for reads; writes use
90-
/// the setter overload.
89+
/// @note Intended as a fast-path check. The underlying storage is not
90+
/// atomic; concurrent reads and writes constitute a data race under
91+
/// the C++ memory model. This is a known legacy condition in the
92+
/// existing code. Writes use the setter overload.
9193
static int
9294
enabled(DiagsTagType dtt)
9395
{
@@ -98,8 +100,9 @@ class DiagsConfigState
98100
///
99101
/// @param dtt DiagsTagType_Debug or DiagsTagType_Action.
100102
/// @param new_value New enabled state (0=off, 1=on, 2=override).
101-
/// @post For DiagsTagType_Debug, DbgCtl's static cache is updated atomically
102-
/// so that existing DbgCtl instances reflect the change on the next check.
103+
/// @post For DiagsTagType_Debug, DbgCtl's internal mode flag is updated with
104+
/// relaxed memory order so that existing DbgCtl instances reflect the
105+
/// change on the next check (no ordering guarantee with other stores).
103106
/// @note Not internally synchronized; call under the Diags internal lock or
104107
/// during single-threaded initialization.
105108
static void enabled(DiagsTagType dtt, int new_value);
@@ -157,19 +160,22 @@ class Diags : public DebugInterface
157160

158161
/// Primary diagnostics log file (e.g., diags.log).
159162
/// May be nullptr if no diagnostics log was configured or if the file could
160-
/// not be opened at initialization. Replaced atomically under the internal
161-
/// lock during log rotation and reseat operations. Do not cache this pointer
162-
/// across calls that may trigger rotation.
163+
/// not be opened at initialization. The pointer swap during log rotation and
164+
/// reseat is performed under the internal lock, but the pre-swap reads are
165+
/// not — see reseat_diagslog() and should_roll_diagslog() for the known
166+
/// concurrency limitations. Do not cache this pointer.
163167
BaseLogFile *diags_log;
164168

165-
/// stdout redirect log file. Never nullptr after construction.
166-
/// Replaced atomically under the internal lock during set_std_output()
167-
/// and output log rolling. Do not cache this pointer.
169+
/// stdout redirect log file. Initialized to a non-null value during
170+
/// construction, but may be set to nullptr by set_std_output() if the
171+
/// redirect target cannot be opened. The pointer swap is performed under
172+
/// the internal lock. Do not cache this pointer.
168173
BaseLogFile *stdout_log;
169174

170-
/// stderr redirect log file. Never nullptr after construction.
171-
/// Replaced atomically under the internal lock during set_std_output()
172-
/// and output log rolling. Do not cache this pointer.
175+
/// stderr redirect log file. Initialized to a non-null value during
176+
/// construction, but may be set to nullptr by set_std_output() if the
177+
/// redirect target cannot be opened. The pointer swap is performed under
178+
/// the internal lock. Do not cache this pointer.
173179
BaseLogFile *stderr_log;
174180

175181
/// Compile-time invariant constant (0x12345678). Used for debug validation.
@@ -227,7 +233,9 @@ class Diags : public DebugInterface
227233
/// @param mode DiagsTagType_Debug or DiagsTagType_Action.
228234
/// @return true if output for mode is enabled (globally or via IP override).
229235
/// @note This is the fast-path check. It does not perform tag matching.
230-
/// Thread-safe; reads atomic state without acquiring the internal lock.
236+
/// Does not acquire the internal lock. Reads DiagsConfigState::enabled,
237+
/// which has a known data race with concurrent writes (see
238+
/// DiagsConfigState::enabled note).
231239
bool
232240
on(DiagsTagType mode = DiagsTagType_Debug) const
233241
{
@@ -254,8 +262,8 @@ class Diags : public DebugInterface
254262
///
255263
/// @param tag Tag string to match. If nullptr, returns true unconditionally.
256264
/// @param mode DiagsTagType_Debug or DiagsTagType_Action.
257-
/// @return true if tag matches the compiled regex, or if no regex is set, or
258-
/// if tag is nullptr.
265+
/// @return true if tag is nullptr, or if tag matches the compiled regex.
266+
/// false if no regex is set, or if tag does not match.
259267
/// @note Thread-safe. The internal lock is acquired to snapshot the regex
260268
/// pointer, then released before the regex is executed.
261269
bool tag_activated(const char *tag, DiagsTagType mode = DiagsTagType_Debug) const;
@@ -307,17 +315,17 @@ class Diags : public DebugInterface
307315
/// @brief Core variadic implementation for diagnostic output.
308316
///
309317
/// Formats and writes the message to all outputs configured for level.
310-
/// Syslog writes occur after the internal lock is released.
311318
///
312319
/// @param tag Optional tag label. May be nullptr.
313320
/// @param level Diagnostic severity level. Must be < DiagsLevel_Count.
314321
/// @param loc Optional source location. May be nullptr.
315322
/// @param fmt printf-style format string. Must not be nullptr.
316323
/// @param ap Argument list for fmt.
317324
/// @pre level < DiagsLevel_Count.
318-
/// @post Message is written to all enabled outputs for level. Syslog, if
319-
/// configured, is written outside the internal lock.
320-
/// @note Thread-safe. Acquires the internal lock for file writes.
325+
/// @post Message is written to all enabled outputs for level.
326+
/// @note Thread-safe. Acquires the internal lock for file writes. On most
327+
/// platforms the lock is released before syslog writes; on FreeBSD the
328+
/// lock is held across syslog as well.
321329
void print_va(const char *tag, DiagsLevel level, const SourceLocation *loc, const char *fmt, va_list ap) const override;
322330

323331
/// @brief Conditionally outputs a diagnostic message if tag is enabled.
@@ -424,16 +432,17 @@ class Diags : public DebugInterface
424432
/// @note Thread-safe. Acquires the internal lock for the pointer reset.
425433
void deactivate_all(DiagsTagType mode = DiagsTagType_Debug);
426434

427-
/// @brief Opens blf and assigns it as the diagnostics log.
435+
/// @brief Opens blf for writing.
428436
///
429437
/// @param blf A BaseLogFile pointing to the desired log path, or nullptr
430-
/// to leave the diagnostics log unchanged.
438+
/// (treated as a no-op).
431439
/// @return true if blf was nullptr or was opened successfully.
432440
/// false if blf could not be opened (blf is deleted and an error
433441
/// is written to stderr).
434442
/// @pre If blf is non-null, it must not already be open.
435-
/// @post On success, diags_log points to the opened blf.
436-
/// On failure, diags_log is unchanged and blf is freed.
443+
/// @post On success, blf is open and ready for writes. The caller is
444+
/// responsible for assigning blf to diags_log.
445+
/// On failure, blf is freed; diags_log is unchanged.
437446
/// @note Thread safety: intended for single-threaded initialization.
438447
/// For runtime reseat, use reseat_diagslog() instead.
439448
bool setup_diagslog(BaseLogFile *blf);
@@ -500,8 +509,8 @@ class Diags : public DebugInterface
500509
/// @return true if any output log was rolled. false otherwise.
501510
/// @note Pointer swaps are performed under the internal lock via
502511
/// set_std_output(). File inspection (fstat, fflush) occurs outside
503-
/// the lock. Callers must ensure no concurrent set_std_output() or
504-
/// reseat_diagslog() call is in progress.
512+
/// the lock. Callers must ensure no concurrent set_std_output() call
513+
/// is in progress.
505514
bool should_roll_outputlog();
506515

507516
/// @brief Redirects stdout or stderr to the specified file.

0 commit comments

Comments
 (0)