Skip to content

Commit 75515ca

Browse files
committed
fix(plan, symbol_provision): a build program's objects stay in this package's images, and vague linkage is not a second provider
Two defects that serving the SYCL runtime's OpenCL adapter made active. Both were latent, neither is caused by #596, and both were found by the checks this branch sharpened rather than by reading. A BUILD PROGRAM'S OBJECTS REACHED A DEPENDENCY'S IMAGE. `role = "object"` with no named target attaches to "every linked image", and that read as "every link unit in this plan" -- which includes the shared library a dependency contributes. Measured: `compat:opencl`'s ICD loader, a C library, came out of the link carrying `saxpy_device` and thirty-seven `sycl::` instantiations, 193 dynamic symbols where its own API is 154, and the process held two copies of the device island. `LinkUnit::dependencyOwned` now separates the images this package produces from the ones a dependency contributes. After: 0 sycl symbols, 154 exports, all of them its own `cl*` entry points. Latent until a SYCL project first had a shared dependency, which is what declaring `compat:opencl` did. VAGUE LINKAGE IS NOT A SECOND PROVIDER. `DynamicSymbol` recorded the symbol's TYPE and not its BINDING, so template instantiations, inline functions and vtables -- which the C++ ABI emits into every image and expects the loader to unify -- were counted as a second provider. `hide_static_cxx_runtime` had already written that rule in a comment; nothing enforced it one layer up. The binding is recorded now, weak definitions are counted rather than reported, and the count is printed for the reason every denominator in this area is printed. Both were invisible while the same artifact still had 68 real findings on top of them. One test moved from positional to designated initialisation: adding a field to `Conflict` bound the provider list to a bool -- a string literal converts to one, so it compiled and the list silently became empty.
1 parent 4f4cff0 commit 75515ca

8 files changed

Lines changed: 178 additions & 6 deletions

File tree

.agents/docs/2026-09-09-dlopen-surface-and-two-unwinders.md

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -616,6 +616,36 @@ predates it, the example's README announces it, and the example runs correctly
616616
across it every time nothing throws. #596 is simply the first time something
617617
threw.
618618

619+
## 8.5 What serving the OpenCL adapter uncovered
620+
621+
R7's reversal put a shared library into every SYCL project's plan for the first
622+
time, and two latent defects became active the moment it did. Both are repaired
623+
here; neither is caused by #596, and neither would have been found by reading.
624+
625+
**A build program's objects reached a dependency's image.** `role = "object"`
626+
with no named target attaches to "every linked image", and that was reading as
627+
"every link unit in this plan" -- which includes the shared library a
628+
dependency contributes. Measured: `compat:opencl`'s ICD loader, a C library,
629+
came out of the link with `saxpy_device` and thirty-seven `sycl::`
630+
instantiations in it, 193 dynamic symbols where its own API is 154, and the
631+
process held two copies of the island. `LinkUnit::dependencyOwned` now says
632+
which images are this package's, and the rule reads "every image THIS PACKAGE
633+
produces".
634+
635+
**The symbol-provision check could not tell a weak definition from a strong
636+
one.** `DynamicSymbol` recorded the type and not the binding, so vague-linkage
637+
definitions -- template instantiations, inline functions, vtables, which the
638+
C++ ABI emits into every image and expects the loader to unify -- were counted
639+
as a second provider. `hide_static_cxx_runtime`'s own comment had already
640+
written the rule ("must not ... unifying those across the process is the
641+
intended C++ ABI behaviour"); nothing enforced it one layer up. The binding is
642+
recorded now, weak definitions are counted rather than reported, and the count
643+
is printed so "clean" cannot read as "did not look".
644+
645+
Both were invisible before because the same artifact had 68 real findings
646+
sitting on top of them. A check whose noise is repaired shows what the noise
647+
was covering, which is the third time this issue has produced that shape.
648+
619649
## 9. What this touches, across the three repositories
620650

621651
The review that asks the other question: not "is each repair right" but "what

CHANGELOG.md

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,26 @@ libgcc_s 本来就在进程里(libstdc++ 需要它),所以这一步只点名一
5656
`12 24 36 48`**顺带推翻了示例里一条写下来的结论** —— 设备镜像不匹配的那次抛出
5757
并非拦不住,它拦不住只是因为 unwinder 是坏的。
5858

59+
### 构建程序的对象不再进入依赖的映像
60+
61+
`role = "object"` 的 action 在没有点名 target 时按「每一个链接映像」附着,而这句
62+
一直被读成「计划里的每一个链接单元」—— 其中包含**依赖贡献的共享库**。实测:
63+
`compat:opencl` 的 ICD loader(一个纯 C 库)链完带着 `saxpy_device` 和 37 个
64+
`sycl::` 实例化,导出 193 个符号而它自己的 API 只有 154 个,进程里于是有两份岛。
65+
`LinkUnit::dependencyOwned` 现在区分「本包产出的映像」与「依赖贡献的映像」。
66+
67+
这条一直潜伏着:直到 SYCL 工程第一次有了共享依赖才被激活。
68+
69+
### 重复符号检查按符号绑定分流
70+
71+
`DynamicSymbol` 只记了类型不记绑定,于是 vague-linkage 定义(模板实例、inline
72+
函数、vtable —— C++ ABI 要求每个映像各发一份、由加载器统一)被算成了第二个提供者。
73+
`hide_static_cxx_runtime` 的注释早就写下了这条规则,但上一层没有任何东西执行它。
74+
现在绑定被记录,weak 定义只计数不算冲突,而且**计数会被打印** —— 否则「干净」读起来
75+
就等于「没看」。
76+
77+
同样是被那 68 条真冲突盖住的:修好噪声之后才看得见噪声盖住了什么。
78+
5979
### 重复符号警告会说出 unwinder 的真实后果
6080

6181
该警告此前把后果一律描述为「库自己的那份副本不会被调用」。对 `_Unwind_*` 这一族,

src/build/plan.cppm

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,19 @@ struct CompileUnit {
8080
struct LinkUnit {
8181
std::string targetName;
8282
enum Kind { Binary, StaticLibrary, SharedLibrary, TestBinary } kind = Binary;
83+
// Does this image belong to a DEPENDENCY rather than to the package being
84+
// built? A `kind = "shared"` dependency contributes a link unit to the
85+
// consumer's plan, and that unit is not one of the consumer's targets.
86+
//
87+
// It decides where a `role = "object"` action's outputs go. "Every linked
88+
// image" is the right default for a build program's objects -- a test
89+
// binary and a static library both need them -- but it was reading as
90+
// "every link unit in the plan", so a device island was linked into a
91+
// dependency's shared library as well. Measured: `compat:opencl`'s ICD
92+
// loader, a C library, came out carrying `saxpy_device` and thirty-seven
93+
// `sycl::` instantiations, and the process then had two copies of the
94+
// island. Latent until a SYCL project first had a shared dependency.
95+
bool dependencyOwned = false;
8396
// Normally relative to plan.outputDir. A `role = "object"` action's outputs
8497
// land here ABSOLUTE, on purpose: ninja identifies a file by the string an
8598
// edge declares, and the action edge declares whatever prepare_actions
@@ -1676,6 +1689,7 @@ make_plan(const mcpp::manifest::Manifest& manifest,
16761689
LinkUnit lu;
16771690
lu.targetName = dep.target.name;
16781691
lu.kind = LinkUnit::SharedLibrary;
1692+
lu.dependencyOwned = true;
16791693
lu.output = dep.output;
16801694
lu.importLibrary = import_library_for(dep.target, naming);
16811695
if (msvcTarget && !lu.importLibrary.empty())

src/build/prepare.cppm

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10358,10 +10358,21 @@ prepare_build(bool print_fingerprint,
1035810358
// consumes `lu.objects`, so the objects an action produced
1035910359
// belong there for exactly the reason a compiled `.cpp`'s do:
1036010360
// the target's content is what it was told to contain.
10361-
const bool image = lu.kind == mcpp::build::LinkUnit::Binary
10361+
//
10362+
// AND NOT A DEPENDENCY'S IMAGE. "Every linked image" means
10363+
// every image THIS PACKAGE produces; a `kind = "shared"`
10364+
// dependency contributes a link unit to this plan and is not
10365+
// one of them. Without the qualifier the SYCL example's device
10366+
// island was linked into `compat:opencl`'s ICD loader as well
10367+
// -- a C library carrying `saxpy_device` -- and the process
10368+
// held two copies of it. An action that means to reach a
10369+
// dependency's target cannot: it is not this package's to
10370+
// fill, and naming it explicitly already fails as unknown.
10371+
const bool image = !lu.dependencyOwned
10372+
&& (lu.kind == mcpp::build::LinkUnit::Binary
1036210373
|| lu.kind == mcpp::build::LinkUnit::SharedLibrary
1036310374
|| lu.kind == mcpp::build::LinkUnit::StaticLibrary
10364-
|| lu.kind == mcpp::build::LinkUnit::TestBinary;
10375+
|| lu.kind == mcpp::build::LinkUnit::TestBinary);
1036510376
const bool wanted = a.targets.empty()
1036610377
? image
1036710378
: std::find(a.targets.begin(), a.targets.end(),

src/build/runtime_validation.cppm

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1036,7 +1036,21 @@ check_symbol_provision(const mcpp::build::BuildPlan& plan,
10361036
.defines = names,
10371037
});
10381038
}
1039-
report.conflicts = sp::conflicting_exports(*exported, providers);
1039+
// WEAK DEFINITIONS ARE COUNTED, NOT REPORTED.
1040+
//
1041+
// A template instantiation or an inline function is emitted into every
1042+
// image that needs it and the loader keeps one; that is the C++ ABI
1043+
// working. Reporting it names a correct build. Measured on the SYCL
1044+
// example once the real findings were repaired: thirty-nine shared
1045+
// symbols remained and thirty-seven were `sycl::queue` and
1046+
// `sycl::buffer` instantiations from the same headers libsycl was
1047+
// built from -- and the other two were the island's own `extern "C"`
1048+
// entry points, which libsycl does not define at all.
1049+
auto all = sp::conflicting_exports(*exported, providers);
1050+
for (auto const& conflict : all)
1051+
if (conflict.isWeak) ++report.sharedWeak;
1052+
std::erase_if(all, [](auto const& c) { return c.isWeak; });
1053+
report.conflicts = std::move(all);
10401054
report.status = report.conflicts.empty() ? sp::Status::Clean
10411055
: sp::Status::Conflict;
10421056
findings.push_back({artifact, std::move(report)});

src/build/symbol_provision.cppm

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,11 @@ export namespace mcpp::build::symbol_provision {
5555
struct Export {
5656
std::string name;
5757
bool isFunc = false;
58+
// A vague-linkage definition (STB_WEAK): a template instantiation, an
59+
// inline function, a vtable. Every translation unit that needs one emits
60+
// it and the loader unifies them; that is the C++ ABI working, not an
61+
// image displacing a library's own copy.
62+
bool isWeak = false;
5863
};
5964

6065
// One object that could also supply a symbol, as the report will name it.
@@ -67,6 +72,7 @@ struct Provider {
6772
struct Conflict {
6873
std::string name;
6974
bool isFunc = false;
75+
bool isWeak = false;
7076
std::vector<std::string> alsoProvidedBy;
7177
};
7278

@@ -92,6 +98,14 @@ struct Report {
9298
std::size_t exported = 0;
9399
std::size_t total = 0;
94100
std::vector<Conflict> conflicts;
101+
// Shared vague-linkage definitions, counted and not listed.
102+
//
103+
// They are NOT a finding: the C++ ABI emits a template instantiation into
104+
// every image that needs it and expects the loader to keep one. Counting
105+
// them is still worth doing -- a reader who runs `nm -D` sees them and has
106+
// to be told which ones this check decided about, or "clean" reads as
107+
// "did not look".
108+
std::size_t sharedWeak = 0;
95109
// Why, for the two non-answers. Empty for Clean and Conflict.
96110
std::string reason;
97111

@@ -169,7 +183,8 @@ exported_definitions(const mcpp::platform::elf::DynamicSymbols& symbols) {
169183
// share an address with relocated data from being excused.
170184
if (!symbol.isFunc && symbols.copyRelocations.contains(symbol.value))
171185
continue;
172-
out.push_back(Export{ .name = symbol.name, .isFunc = symbol.isFunc });
186+
out.push_back(Export{ .name = symbol.name, .isFunc = symbol.isFunc,
187+
.isWeak = symbol.isWeak });
173188
}
174189
std::ranges::sort(out, {}, &Export::name);
175190
return out;
@@ -179,7 +194,8 @@ std::vector<Conflict> conflicting_exports(std::span<const Export> exports,
179194
std::span<const Provider> closure) {
180195
std::vector<Conflict> out;
181196
for (auto const& exported : exports) {
182-
Conflict conflict{ .name = exported.name, .isFunc = exported.isFunc };
197+
Conflict conflict{ .name = exported.name, .isFunc = exported.isFunc,
198+
.isWeak = exported.isWeak };
183199
for (auto const& provider : closure) {
184200
if (std::ranges::find(provider.defines, exported.name)
185201
!= provider.defines.end())
@@ -223,6 +239,13 @@ std::string Report::explain(std::string_view artifact) const {
223239

224240
body += " Also provided by:\n";
225241
for (auto const& label : providers) body += std::format(" {}\n", label);
242+
if (sharedWeak > 0)
243+
body += std::format(
244+
" ({} vague-linkage definition{} -- template instantiations, inline\n"
245+
" functions, vtables -- {} also shared and are NOT part of this\n"
246+
" finding: the C++ ABI emits one per image and the loader keeps one.)\n",
247+
sharedWeak, sharedWeak == 1 ? "" : "s",
248+
sharedWeak == 1 ? "is" : "are");
226249

227250
// WHY it matters, then what to do — IN THE ORDER THAT ACTUALLY WORKS.
228251
//

src/runtime/elf.cppm

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,13 @@ struct DynamicSymbol {
137137
// dynamic symbol table therefore has exactly one cause — the linker
138138
// exported it so that some shared object's reference would bind to it.
139139
bool isFunc = false;
140+
// STB_WEAK. A vague-linkage definition -- a template instantiation, an
141+
// inline function, a vtable -- which the C++ ABI emits into every
142+
// translation unit that needs it and expects the loader to unify across
143+
// the process. That is the intended behaviour, not a leak, so a caller
144+
// asking "is this image providing something twice" has to be able to tell
145+
// it from a strong definition that displaces a library's own.
146+
bool isWeak = false;
140147
std::uint64_t value = 0; // st_value; the key a copy relocation matches
141148
};
142149

@@ -274,6 +281,7 @@ constexpr std::uint64_t kDtGnuHash = 0x6ffffef5;
274281
constexpr std::uint64_t kSymEntrySize = 24;
275282
constexpr std::uint64_t kRelaEntrySize = 24;
276283

284+
constexpr unsigned char kStbWeak = 2;
277285
constexpr unsigned char kSttObject = 1;
278286
constexpr unsigned char kSttFunc = 2;
279287
constexpr unsigned char kSttGnuIfunc = 10;
@@ -731,6 +739,7 @@ inspect_dynamic_symbols(const std::filesystem::path& object) {
731739
out.defined.push_back(DynamicSymbol{
732740
.name = std::move(*name),
733741
.isFunc = (type == detail::kSttFunc || type == detail::kSttGnuIfunc),
742+
.isWeak = (bind == detail::kStbWeak),
734743
.value = *value,
735744
});
736745
}

tests/unit/test_symbol_provision.cpp

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,8 +137,14 @@ TEST(SymbolProvision, TheReportNamesEveryProviderAndCapsTheSymbolList) {
137137
report.status = sp::Status::Conflict;
138138
report.total = 217;
139139
for (int i = 0; i < 20; ++i)
140+
// DESIGNATED, not positional. A field added to `Conflict` between
141+
// `isFunc` and `alsoProvidedBy` bound the provider list to a bool
142+
// here -- a string literal converts to one, so it compiled, and the
143+
// provider list silently became empty.
140144
report.conflicts.push_back(sp::Conflict{
141-
std::format("sym{}", i), true, {"/pkg/lib/libz.so.1"}});
145+
.name = std::format("sym{}", i),
146+
.isFunc = true,
147+
.alsoProvidedBy = {"/pkg/lib/libz.so.1"}});
142148
report.exported = report.conflicts.size();
143149

144150
auto text = report.explain("consumer");
@@ -199,3 +205,48 @@ TEST(SymbolProvision, OrdinaryLinkFlagsDoNotVoidThePredicate) {
199205
"-O2", "-Wl,-rpath,$ORIGIN", "-lz", "-Wl,--as-needed",
200206
"-Wl,--enable-new-dtags", "-static-libstdc++", "-shared"}));
201207
}
208+
209+
// ── vague linkage is not a second provider ─────────────────────────────────
210+
//
211+
// A template instantiation, an inline function or a vtable is emitted into
212+
// every image that needs it and the loader keeps one. That is the C++ ABI
213+
// working, and reporting it names a correct build.
214+
//
215+
// Measured on the SYCL example once the real findings were repaired: of the
216+
// thirty-nine symbols the image still shared with `libsycl.so.9`,
217+
// thirty-seven were `sycl::queue` and `sycl::buffer` instantiations from the
218+
// same headers libsycl was built from -- and the remaining two were the
219+
// island's own `extern "C"` entry points, which libsycl does not define. A
220+
// check that could not tell binding from name reported all of them.
221+
222+
TEST(SymbolProvision, AWeakDefinitionIsCarriedThroughAsWeak) {
223+
auto s = image();
224+
auto weak = func("_ZN4sycl3_V15queueD2Ev");
225+
weak.isWeak = true;
226+
s.defined.push_back(weak);
227+
s.defined.push_back(func("saxpy_device"));
228+
auto exports = sp::exported_definitions(s);
229+
ASSERT_TRUE(exports.has_value());
230+
ASSERT_EQ(exports->size(), 2u);
231+
// Sorted by name: the mangled one first.
232+
EXPECT_TRUE((*exports)[0].isWeak);
233+
EXPECT_FALSE((*exports)[1].isWeak);
234+
}
235+
236+
TEST(SymbolProvision, AConflictRemembersWhetherItsDefinitionIsWeak) {
237+
std::vector<sp::Export> exports{
238+
{ .name = "_ZN4sycl3_V15queueD2Ev", .isFunc = true, .isWeak = true },
239+
{ .name = "inflate", .isFunc = true, .isWeak = false },
240+
};
241+
std::vector<sp::Provider> closure{
242+
{ .label = "libsycl.so.9",
243+
.defines = {"_ZN4sycl3_V15queueD2Ev", "inflate"} },
244+
};
245+
auto conflicts = sp::conflicting_exports(exports, closure);
246+
ASSERT_EQ(conflicts.size(), 2u);
247+
// Both are shared; only the binding separates them, and the caller is what
248+
// decides which one is a finding. Asserted here rather than in the caller
249+
// so the DATA carries the distinction even if a future caller forgets it.
250+
EXPECT_TRUE(conflicts[0].isWeak);
251+
EXPECT_FALSE(conflicts[1].isWeak);
252+
}

0 commit comments

Comments
 (0)