Skip to content

[Bughunt Phase 3] Architectural risk reduction: 1.15MB zero-filled RDD table, GPL-incompatible Numerical Recipes code, no error channel #293

Description

@grzanka

Found during a full bughunt/architecture review. These are structural issues rather than single-line bugs — each needs the Phase 1 test suite in place first so the refactor can be verified against pinned output, hence sequencing this after CI hardening.

1. AT_RDD_Tabulated.h is a 1.37MB header that is entirely zeros

include/AT_RDD_Tabulated.h is 8145 lines / 1,366,844 bytes. It defines static const AT_TAB_RDD_struct AT_RadDiff_RDD, labelled in-file as:

/** PROPRIETARY DATA --- NOT TO BE COMMITTED INTO PUBLIC SVN
    BEGIN */

I parsed all 79,152 numeric literals in the table body: every single one is exactly 0.0. The underlying data was evidently stripped before commit, but the multi-megabyte placeholder shell was committed anyway — to a public repo, contradicting its own comment.

Measured impact: nm -S on the built libamtrack.so shows AT_RadDiff_RDD alone occupies 1,207,040 bytes — about 78% of the entire 1.55MB shared library — duplicated across the object files that include it. AT_RDD.h and AT_ElectronRange.h both #include this header, so it's parsed into nearly every translation unit and shipped as an installed public header.

The model it backs (RDD model 8, "Radical Diffusion [Andrea Mairani]") is functionally dead: AT_D_RDD_Gy rejects it against every ER model I tried, including its own purpose-built match (ER model 9, "ER model for Andrea Mairani's radical diffusion RDD") — confirmed via example/basic_plots -t RDD -s 8 -y 9, which prints Incompatible ER model ... used with RDD model ... and returns nothing.

This significantly overlaps #57 ("Remove unnecessary items from header files").

Proposed fix: delete the inline table; if the model is wanted later, load it from an external data file at runtime (so a public repo never needs to carry proprietary numbers as source), or remove the model entirely until real data is available.

2. Numerical Recipes code creates a licence conflict with GPL-3

src/AT_NumericalRoutines.c contains gammln, nrerror, zriddr, locate, locate_index_in_2d_table, and a cubic spline, with header comments explicitly stating provenance:

* Numerical Recipes: Logarithm of gamma function
* Numerical Recipes: standard error handler
* From Numerical Recipes in C, 2nd ed., 1992:
* interpolation on a table: code (w/ adapted indices) from Numerical Recipes, 2rd ed., chapter 3.1
* Created on basis of Numerical Recipes in fortran 77: ... chapter 3.3 Cubic Spline Interpolation

The NR licence restricts redistribution and is not compatible with GPL-3, under which this library is distributed. This has been open as a concern since #97 (2020) with no resolution.

The good news: GSL is already a hard build dependency (find_package(GSL REQUIRED) in CMakeLists.txt) and already provides a drop-in replacement for every one of these: gsl_sf_lngamma (for gammln), gsl_root_fsolver_brent (for zriddr), gsl_interp_bsearch (for locate), gsl_spline/gsl_interp_cspline (for the cubic spline routines). This is a rare refactor that both resolves a legal blocker and net-deletes code.

Separately, pbdv_/dvsa_/dvla_/vvla_ in the same file are f2c-translated CERNLIB/SPECFUN routines with their own provenance that should be documented (a short PROVENANCE.md noting origin and licence compatibility for each non-original numerical routine would help future reviewers, including future instances of this exact audit).

3. No usable error channel across most of the public API

AT_Error.h defines enum AT_error_no, enum AT_energy_ranges, and a message table, but is #included by no file except AT_Error.c itself — it's dead infrastructure. Meanwhile, of 382 public functions: 182 return double (no room to signal failure), 85 return void. Failures currently surface as 0.0, -1, or a printf guarded by #ifndef NDEBUG — which means diagnostics are compiled out entirely in Release builds, the configuration CI actually ships (cmake .. -DCMAKE_BUILD_TYPE=Release in all three reusable workflows).

Concretely, six public energy-loss functions currently have their real implementation commented out and unconditionally return 0.0;: AT_Vavilov_FWHM, AT_energy_loss_FWHM, AT_energy_loss_keV_Landau_FWHM, AT_energy_loss_keV_Landau_Mode, AT_energy_loss_keV_Vavilov_FWHM, AT_energy_loss_keV_Vavilov_Mode (src/AT_EnergyLoss.c, lines ~509–583). A caller gets a plausible-looking zero, not an error.

Also: static const AT_error_msg error_messages[7] is defined in the header, sized by a hand-maintained literal 7 that will silently desync from enum AT_error_no the next time an error code is added, and gets duplicated into every translation unit that includes it (currently none, but that's the point of fixing #1 above).

Proposed fix (incremental, not a 382-function rewrite):

  • Move error_messages[] out of the header into AT_Error.c, generated/sized from the enum rather than a magic number
  • Wire up AT_Error.h — start by having the six stubbed-out functions above return a documented sentinel and log via a pluggable callback (not #ifndef NDEBUG printf) instead of a bare 0.0
  • Establish (and document in a contributor guide) the convention for new and touched code going forward: int AT_x(..., double *out) returning an AT_error_no, rather than attempting to retrofit all 382 existing signatures
  • Replace #ifndef NDEBUG printf(...) diagnostic sites with the same pluggable log callback so Release builds aren't silent

4. CMake modernization (lower priority, bundle with the above)

  • file(GLOB SOURCE_FILES "src/*.c") / HEADER_FILES (CMakeLists.txt:89-90) doesn't use CONFIGURE_DEPENDS, so adding a new source file doesn't trigger reconfiguration
  • find_package(GSL REQUIRED) immediately followed by a dead if (NOT GSL_FOUND) block (REQUIRED already aborts configure on failure) — CMakeLists.txt:81-84
  • add_library(amtrack SHARED ...) hardcodes SHARED; consider BUILD_SHARED_LIBS so static linking (useful for the WASM build path) doesn't need a separate CMake invocation pattern
  • libamtrackConfig.cmake is hand-written via file(WRITE ...) containing a literal @PACKAGE_INIT@ that is never expanded because it isn't run through configure_package_config_file — the installed CMake package config is subtly broken for downstream find_package(libamtrack) consumers

Acceptance criteria

  • AT_RadDiff_RDD's inline zero table is removed from the public header; libamtrack.so shrinks accordingly
  • No Numerical Recipes–derived code remains in src/AT_NumericalRoutines.c; GSL equivalents pass the Phase 1 characterization tests
  • The six stubbed energy-loss functions no longer return a bare 0.0 on the un-implemented path
  • libamtrackConfig.cmake is generated via configure_package_config_file and @PACKAGE_INIT@ is actually expanded

Related: #57, #97

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions