Architecture parsers are dropped when Core is linked as a static library

Open
#327 0 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
55/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Active
Tech stack
cmake, cpp
Domain
build-system

Research direction

Start with get_dsp.cpp and the parser translation units named in the report: container.cpp, convnet.cpp, linear.cpp, lstm.cpp, sequential.cpp, and wavenet/model.cpp. Reproduce the direct-object and libnam.a link commands on a supported platform, then trace which parser registration symbols are retained. Done means the chosen fix makes a static-library consumer load the A2 model with its parser registered, without relying on undocumented platform-specific linker flags.

Written by the indexing model from the issue text.

Description

Summary

When Core is compiled into a static library and linked into a consumer, every
architecture parser silently disappears. Loading any model then fails with:

No config parser registered for architecture: SlimmableContainer

The parsers are registered from static initializers, one per translation unit
(container.cpp, convnet.cpp, linear.cpp, lstm.cpp, sequential.cpp,
wavenet/model.cpp). A linker only pulls an archive member in when something
already in the link references one of its symbols. Nothing references those TUs
— the registration is the whole point — so they are dropped, and their
initializers never run.

Upstream's own CMake never hits this: tools/CMakeLists.txt passes the sources
directly to add_executable, so every object is always in the link. The bug only
shows up for consumers who build Core as a library, which is the normal way to
embed it.

Reproduction

Same objects, two link strategies. Only the second one fails.

git clone --recurse-submodules https://github.com/sdatkinson/NeuralAmpModelerCore.git
cd NeuralAmpModelerCore

cat > /tmp/main.cpp <<'CPP'
#include <iostream>
#include "NAM/get_dsp.h"
int main(int argc, char** argv) {
  try {
    auto dsp = nam::get_dsp(std::filesystem::path(argv[1]));
    std::cout << "loaded OK, expected sample rate = " << dsp->GetExpectedSampleRate() << "\n";
    return 0;
  } catch (const std::exception& e) {
    std::cout << "FAILED: " << e.what() << "\n";
    return 1;
  }
}
CPP

INC="-I. -IDependencies/eigen -IDependencies/nlohmann"
FLAGS="-std=c++20 -O2 -DNAM_ENABLE_A2_FAST"
mkdir -p obj
for f in NAM/*.cpp NAM/wavenet/*.cpp; do
  c++ $FLAGS $INC -c "$f" -o "obj/$(basename "${f%.cpp}").o"
done

# A) objects linked directly — this is what upstream's CMake does
c++ $FLAGS $INC /tmp/main.cpp obj/*.o -o direct
./direct example_models/A2.nam

# B) exact same objects, but through an archive
ar rcs libnam.a obj/*.o
c++ $FLAGS $INC /tmp/main.cpp libnam.a -o static
./static example_models/A2.nam

Observed on macOS 15 with AppleClang:

── A) sources linked directly (what upstream's CMake does) ──
loaded OK, expected sample rate = 48000

── B) same objects, but via a static library ──
FAILED: No config parser registered for architecture: SlimmableContainer

Reproduced on v0.5.4. The registration mechanism is unchanged on main
(2563c0f), which adds a sixth registrar in sequential.cpp.

Workaround for consumers

Force the archive members in. This is what we ended up doing, because it is the
only form that is identical on the three desktop platforms — --whole-archive,
-force_load and /WHOLEARCHIVE all spell it differently:

using ConfigParser = std::unique_ptr<nam::ModelConfig> (*)(const nlohmann::json&, double);
volatile ConfigParser g_anchor = nullptr;

void anchor_parsers() {
  g_anchor = &nam::container::create_config;
  g_anchor = &nam::convnet::create_config;
  g_anchor = &nam::linear::create_config;
  g_anchor = &nam::lstm::create_config;
  g_anchor = &nam::wavenet::create_config;
}

Note that a const array of the same pointers does not work: the compiler
discards it as unused before the linker ever sees the references. The writes have
to be volatile.

Suggested fix

Any of these would remove the trap; the second seems cheapest:

  1. Export a nam::register_builtin_parsers() that consumers call once, and
    document it.
  2. Reference the create_config functions from get_dsp.cpp. That TU is always
    linked, since it defines the entry point everyone calls, so it drags the rest
    in without any consumer-visible API change.
  3. Document the --whole-archive / -force_load / /WHOLEARCHIVE requirement in
    the README.

The current failure mode is quiet and points in the wrong direction: the error
says the architecture is not registered, which reads like an unsupported model
rather than a link-time problem.

Dominant language
C++
Stars
931
Forks
178
Avg merge
3h 31m
Merged PRs (30d)
7

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from sdatkinson/NeuralAmpModelerCore

All issues in sdatkinson/NeuralAmpModelerCore

Similar issues

More C++ issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.