Conversation
|
I‘m open to bikeshedding on the name here, by the way. (Or the approach, although this seems like the simplest thing we could do to address the problem for now, even if the compiler error messages thing is unfortunate.) |
|
lgtm, its great that the duplicated sanitize hook in the meta libs can be centralized. I dont have access to a machine to build these changes atm so I wont add the official GH review, but I wouldn't be opposed to having it be merged sooner |
|
I must be misunderstanding something here, I think. How does |
|
I think you’re just forgetting what C++ is like – the headers include tons of code. e.g., in // The _M_get operations have _M_engaged as a precondition.
constexpr _Tp&
_M_get() noexcept
{
__glibcxx_assert(this->_M_is_engaged());
return static_cast<_Dp*>(this)->_M_payload._M_get();
}which calls through to: // Assert.
#ifdef _GLIBCXX_VERBOSE_ASSERT
namespace std
{
#pragma GCC visibility push(default)
// Don't use <cassert> because this should be unaffected by NDEBUG.
extern "C++" _GLIBCXX_NORETURN
void
__glibcxx_assert_fail /* Called when a precondition violation is detected. */
(const char* __file, int __line, const char* __function,
const char* __condition)
_GLIBCXX_NOEXCEPT;
#pragma GCC visibility pop
}
# define _GLIBCXX_ASSERT_FAIL(_Condition) \
std::__glibcxx_assert_fail(__FILE__, __LINE__, __PRETTY_FUNCTION__, \
#_Condition)
#else // ! VERBOSE_ASSERT
# define _GLIBCXX_ASSERT_FAIL(_Condition) __builtin_abort()
#endif
#if defined(_GLIBCXX_ASSERTIONS)
// When _GLIBCXX_ASSERTIONS is defined we enable runtime assertion checks.
// These checks will also be done during constant evaluation.
# define __glibcxx_assert(cond) \
do { \
if (__builtin_expect(!bool(cond), false)) \
_GLIBCXX_ASSERT_FAIL(cond); \
} while (false)which means that programs using |
ConnorBaker
left a comment
There was a problem hiding this comment.
Looks good! Thank you for working on this :)
pkgs/by-name/sa/sanitiseHeaderPathsHook/sanitise-header-paths-hook.bash
Outdated
Show resolved
Hide resolved
alyssais
left a comment
There was a problem hiding this comment.
Makes sense to me now, thanks.
C++ headers often use `__FILE__` in error messages, causing the development outputs of libraries to leak into the runtime closure of packages using them. This hook abstracts away a pattern used in a few places throughout the tree to have headers identify themselves by a sanitized path that does not cause runtime dependencies. This does unfortunately mean that compiler error messages will reference the sanitized path. The only alternatives I can imagine are to patch compilers to handle `__FILE__` specially, or to have libraries propagate a hook that removes references. The latter would potentially need to be propagated recursively due to `#include` semantics and would be less precise than this.
This reduces the WebKitGTK runtime closure size from 1.45 GiB to 1.22 GiB on `aarch64-linux`, as measured by `nix-tree`.
2e39fb4 to
1c2dfb8
Compare
|
I discovered that compilers support the |
|
Hi, this breaks using gdb on code inlined from templates, as the source file is mangled to /nix/store/eeee.... |
|
steps to reproduce the issue: prints /nix/store/eeeeee paths instead of real paths |
|
@trofi’s patch (which I hadn’t seen before) does seem like it should work. But it doesn’t help for Darwin, or apparently for WebKitGTK – both of which use LLVM. So I guess we’d need to patch Clang too, or else find a cleaner way of doing this. |
|
I am going to port the patch to clang. |
|
Could we potentially use the compiler flags instead? I think we could just inject them in It would break for Edit: Hmm, the GCC bug report shows paths to individual header files being listed. Surely since it’s a prefix map we only need to list top‐level store paths? But then later discussion in the issue shows that it might just be slightly too much in general and push us over the limit, blah. In any case I feel like we ought to be able to use the flags for Clang at least, since it probably doesn’t have this limitation. |
|
I'm not sure we lower the maintenance burden by maintaining a patch used only for gcc and cc-wrapper code used only for clang. |
|
Once the GCC bug is fixed, we can use the same code for both. It would just be a couple additional lines in Edit: Though I’m only thinking about the |
|
Here is the patch #424844 |
|
This change has been causing issues in our project which uses CMake/Ninja. For some reason it results in CMake + ninja/make thinking objects are not up-to-date anymore, causing them to be built again in the I have found it quite hard to debug. Ninja logging does not give much information, but Full make output: https://gist.github.com/danieldk/8b445776745bf44721f3de8c28b6cbaf If I undo this patch: danieldk@b8c1118 , the rebuilds in I will try to see if I can make a more minimal example or if I can find a package in nixpkgs that has similar rebuilds in My current theory is that some CMake script uses the C preprocessor to find the header dependencies of a file and this change trips it up, making it use the fake paths rather than the real header paths. |
|
First example I found in nixpkgs: Full log: https://gist.github.com/danieldk/97eceeb9568968b0af290c136d5b78e6
|
|
I created an issue: #428546 , since it's more discoverable than putting comments in a merged PR. Sorry for the noise here! |
C++ headers often use
__FILE__in error messages, causing the development outputs of libraries to leak into the runtime closure of packages using them. This hook abstracts away a pattern used in a few places throughout the tree to have headers identify themselves by a sanitized path that does not cause runtime dependencies.This does unfortunately mean that compiler error messages will reference the sanitized path. The only alternatives I can imagine are to patch compilers to handle
__FILE__specially, or to have libraries propagate a hook that removes references. The latter would potentially need to be propagated recursively due to#includesemantics and would be less precise than this.Adding the hook to GCC (for
libstdc++headers) reduces the WebKitGTK runtime closure size from 1.45 GiB to 1.22 GiB onaarch64-linux, as measured bynix-tree.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.