Re: MIOpen symbols file and libstdc++-dev
Hi Xuanteng,
thank your for the analysis.
On 2024-06-30 05:23, Xuanteng Huang wrote:
> The symbol file generated by dpkg-gensymbols are large with 5000+ exported symbols. MIOpen upstream does not compile the library with hidden visibility as other ROCm packages like rocSPARSE [2]. This is because some MIOpen tests directly use the internal APIs [3]. So if we hide these APIs, the tests will fail to build.
>
> Currently MIOpen use the version script to control which symbols to export [4]. Alternatively, we can add symbols referenced in tests into the “global” field. But I suspect the resulting symbol file will be almost the same large as the current one, as the tests call a wide range of internal APIs (I count the number of "undefined reference to” linking errors during building which is 4000+).
Yeah, that doesn't sound like it would solve a lot. And if the goal is
symbol hygiene, we'd be missing it anyway.
> If the large symbol file and the mixing of public and internal APIs are acceptable, we can continue. Otherwise if we want a clean and precise symbol file with only public interfaces, we may need to remove the -test package or statically link the test executables against MIOpen with a second compilation pass. Do you have any ideas?
Not me, unfortunately.
It seems we have conflicting goals: we want the library package to
export only the public interface, whereas the tests here were designed
to test internal interfaces.
I could be super-pedantic and/or wrong, but my impression was that unit
tests should test only public interfaces. So one theoretical(!) solution
would be to re-write tests such that they use public interfaces that
touch the internal codepaths, another would be to make some of those
internal interfaces public after all. Both of this would be an undue
burden though.
In the end, I think having tests is more important that symbol hygiene,
so if we cannot find a solution, I'd vote for the former.
Nevertheless, I'd really appreciate feedback here, especially if Cory or
Jeremy have thoughts on this.
> Besides, MIOpen currently depends on libstdc++-dev as runtime compilation with hipRTC requires some C++ headers (e.g., <limits>) to compile the HIP kernel codes, otherwise, running autopkgtest will report the error like this:
>
> $ MIOpenDriver reduce -V 1 -t 1
> MIOpen(HIP): Error [Compile] 'hiprtcCompileProgram(prog.get(), c_options.size(), c_options.data())' gridwise_generic_reduction_first_call_multiblock_reduce_partial_dims.cpp: HIPRTC_ERROR_COMPILATION (6)
> MIOpen(HIP): Error [BuildHip] HIPRTC status = HIPRTC_ERROR_COMPILATION (6), source file: gridwise_generic_reduction_first_call_multiblock_reduce_partial_dims.cpp
> MIOpen(HIP): Warning [BuildHip] In file included from /tmp/comgr-8e8b54/input/gridwise_generic_reduction_first_call_multiblock_reduce_partial_dims.cpp:29:
> In file included from /tmp/comgr-8e8b54/include/tensor_descriptor_helper.hpp:4:
> In file included from /tmp/comgr-8e8b54/include/common_header.hpp:10:
> /tmp/comgr-8e8b54/include/data_type.hpp:14:10: fatal error: 'limits' file not found
> 14 | #include <limits> // std::numeric_limits
> | ^~~~~~~~
> 1 error generated when compiling for gfx1030.
>
> Nevertheless, nothing else depends on libstdc++-dev except for gcc/clang packages.
For context: `apt-cache rdepends libstdc++-*-dev`
> As ROCm stack depends on LLVM 17 packages now, the libc++-17-dev package also provides the header file [5]. But it installs into a non-standard path, where hipRTC cannot find during runtime which leads the same error above. Maybe we can choose to add a hardcoded compilation option `-I/usr/lib/llvm-17/include/c++/v1` as we did for GPU architecture [6]?
If it works with libstdc++-dev, then I could be that we might just be
missing some auxiliary package that fixes this (eg symlink non-standard
path to standard).
libstdc++-dev is a purely virtual package provided by some concrete
libstdc++-VERSION-dev. We don't want to depend on that because as you
pointed out, only gcc/clang depend on those; it would be highly
irregular for us to do so.
So what is the package that "fixes" this? I assume that depending
directly on clang-17 would do so, but would this be too big of a dependency?
Slightly tangential: If hipRTC by itself cannot be useful without this,
I guess the dependency should be fixed there.
Best,
Christian
> [1]: https://salsa.debian.org/rocm-team/miopen/-/merge_requests/7
> [2]: https://github.com/ROCm/rocSPARSE/blob/develop/library/CMakeLists.txt#L98
> [3]: https://salsa.debian.org/rocm-team/miopen/-/blob/master/test/activation.cpp?ref_type=heads#L68
> [4]: https://salsa.debian.org/rocm-team/miopen/-/blob/master/src/CMakeLists.txt?ref_type=heads#L724-740
> [5]: https://packages.debian.org/trixie/amd64/libc++-17-dev/filelist
> [6]: https://salsa.debian.org/rocm-team/miopen/-/blob/master/debian/patches/0007-specify-the-gpu-arch-for-hipRTC.patch
>
Reply to: