[Jacobfaib]

Description

closes #12340

Checklist

• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[Jacobfaib]

/ok to test

[Jacobfaib]

/ok to test

[Jacobfaib]

/ok to test

[Jacobfaib]

/ok to test

[Jacobfaib]

/ok to test

[miscco]

The libcu++ part and the general direction looks fine to me.

We should IMHO not annotate test functions, because those should be able to discover issues out users have

[Jacobfaib]

/ok to test

[Jacobfaib]

/ok to test

[Jacobfaib]

/ok to test

[oleksandr-pavlyk]

https://github.com/NVIDIA/cccl/blob/6ebfbc58/thrust/thrust/system/detail/error_category.inl#L274-L284 adds _CCCL_HOST_API to both generic_category() and system_category(). Hidden visibility gives each shared library its own singleton, but category equality compares addresses. These should use _CCCL_PUBLIC_HOST_API. The cuda_category() also needs to be checked.

[oleksandr-pavlyk]

The fix-it here https://github.com/NVIDIA/cccl/blob/6ebfbc58/cmake/clang_tidy_plugins/hide_from_abi/hide_from_abi.cpp#L162 unconditionally inserts _CCCL_HIDE_FROM_ABI which includes inline.

When it processes inline X::X() = default; it becomes _CCCL_HIDE_FROM_ABI inline X::X() = default; and clang errors with -Werror=duplicate-decl-specifier

[oleksandr-pavlyk]

Changes to nvbench_helper/ and cub/benchmarks are fine, but they are not necessary.

Question: Is there reason to include benchmark implementation and nvbench_helper/ in library ABI hygiene checks? Is it just for uniformity of style policy?

[Jacobfaib]

When it processes inline X::X() = default; it becomes _CCCL_HIDE_FROM_ABI inline X::X() = default; and clang errors with -Werror=duplicate-decl-specifier

Yes, this is an unfortunate side effect from Clang explicitly not including storage duration or qualifier (like const or volatile) nodes in the AST. You can match a symbol that is inline (implicitly or explicitly) but you cannot match the literal "inline" word itself. The way to retrieve it is to lexically parse backwards (using clang::Lexer::parseToken()) from the symbol decl to the clang::Token representing it but that is... also not trivial, because you need to know when to stop in order to bound the search.

FWIW we do this lexical search for the other macros, but only if we have a template or requires clause because then we know that the macros/specifiers must be between the end of the template/requires clause and the beginning of the function decl. But for constructors we aren't guaranteed any kind of reasonable stopping point; you cannot, for example assume that the enclosing class decl is your stopping point:

class Foo { <-- class decl
  inline Foo() = default;
         ^
         backward lexical search starts here
};

Because I can define the function out-of-line:

class Foo {
  inline Foo(); // declaration has inline...
};

// potentially hundreds of lines in between

Foo::Foo() = default; // ...which makes this implicitly inline
     ^
     backward lexical search starts here

So... all this is to say, I allowed this particual incorrect substitution because it will already be diagnosed by the regular compiler later on.

[Jacobfaib]

Question: Is there reason to include benchmark implementation and nvbench_helper/ in library ABI hygiene checks? Is it just for uniformity of style policy?

Mostly for uniformity, but it can also reduce binary size since it allows the linker to discard unused internal symbols that the compiler might not otherwise be able to remove.

[github-actions]

🥳 CI Workflow Results

🟩 Finished in 4h 14m: Pass: 100%/502 | Total: 20d 02h | Max: 4h 13m | Hits: 27%/3541281

See results here.