unlock unlimited code runs for $79, limited time →
NOSSLETTER.techLIVE
back to OSSpedia
llvm/llvm-project
senior7/10
api-designtesting-strategycompiler-internals

[Clang] Fix a regression introduced by #216729.

Project: llvm/llvm-project | Language: C++ | Concepts: api-design, testing-strategy, compiler-internals | Difficulty: senior
Link: https://github.com/llvm/llvm-project/pull/219998

The problem

A regression was introduced in the mangling of concept template parameters due to confusion in the NestedNameSpecifier (NNS) API. The interface for NNS constructors was ambiguous, leading to incorrect usage. Additionally, there were no tests verifying the mangling of concept template parameters, making such regressions harder to catch.

Why it's non-trivial

The NestedNameSpecifier class has two constructors that appear similar but have subtly different semantics. This can easily lead to mistakes, especially in a complex codebase like Clang's AST mangler, where the correct representation of names is critical. Naively passing a default-constructed NNS versus an explicit "empty" value can result in different behaviors, and without tests, such issues can go unnoticed.

The approach

1. Fixing the ambiguous constructor usage in the mangler:

The original code called mangleUnresolvedName with a default-constructed NestedNameSpecifier():

-    mangleUnresolvedName(NestedNameSpecifier(), DTI->getName(),
+    mangleUnresolvedName(/*NestedNameSpecifier=*/std::nullopt, DTI->getName(),
                         DTI->template_arguments().data(),
                         DTI->getNumTemplateArgs(), Arity);

Annotation:
This change replaces a default-constructed NestedNameSpecifier() (which represents an "invalid" state) with std::nullopt, making it explicit that there is no nested name. This avoids the confusion caused by the overloaded constructors and ensures the correct mangling behavior for concept template parameters.

2. Adding targeted regression tests:

A new test file verifies the mangling of concept template parameters, including both empty and non-empty cases:

+// CHECK: define {{.*}}@_ZN8GH2188201fITpTtTyEJEEEvvQfraa1CIiE(
+// CHECK: define {{.*}}@_ZN8GH2188201fIJNS_1CEEEEvvQfraa1CIiE(
+template <template <typename> concept... C>
+void f() requires (C<int> && ...) {}
+
+// CHECK: define {{.*}}@_ZN8GH2188201gITpTtTyEJEEEvvQfraa1VIiE(
+// CHECK: define {{.*}}@_ZN8GH2188201gIJNS_1VEEEEvvQfraa1VIiE(
+template <template <typename> auto... V>
+void g() requires (V<int> && ...) {}

Annotation:
This test ensures that both the empty and non-empty cases for concept template parameters are correctly mangled. It uses FileCheck to assert the presence of the expected mangled names in the generated LLVM IR.

From the review thread

zwuis: If I remember correctly they were used for DenseMap (empty keys and tombstone keys). CC @mizvekov

mizvekov: Yes that's correct.

mizvekov: The fix here is correct and self contained enough, we can separate discussion about possible API changes to another medium.

Takeaways

  • When an API exposes multiple constructors with similar signatures but different semantics, it increases the risk of subtle bugs—prefer clearer, less ambiguous interfaces.
  • Regression tests targeting previously untested or edge-case language features (like concept template parameter mangling) are essential for maintaining compiler correctness.
  • Making intent explicit in code (e.g., using std::nullopt instead of a default-constructed object) improves maintainability and reduces the chance of misinterpretation.