Hi Thomas,
On Thu, Jan 14 2021, Thomas Schwinge wrote:
> Hi!
>
> I'm raising here an issue with HSA libgomp plugin code changes from a
> while ago. While HSA is now no longer relevant for GCC master branch,
> the same code has also been copied into the GCN libgomp plugin.
>
> This is commit b8d89b03db5f212919e4571671ebb4f5f8b1e19d (r242749) "Remove
> build dependence on HSA run-time":
>
> On 2016-11-22T14:27:44+0100, Martin Jambor <mjam...@suse.cz> wrote:
>> --- a/libgomp/plugin/configfrag.ac
>> +++ b/libgomp/plugin/configfrag.ac
>
>> @@ -195,8 +183,8 @@ if test x"$enable_offload_targets" != x; then
>> tgt_name=hsa
>> PLUGIN_HSA=$tgt
>> PLUGIN_HSA_CPPFLAGS=$HSA_RUNTIME_CPPFLAGS
>> - PLUGIN_HSA_LDFLAGS="$HSA_RUNTIME_LDFLAGS $HSA_KMT_LDFLAGS"
>> - PLUGIN_HSA_LIBS="-lhsa-runtime64 -lhsakmt"
>> + PLUGIN_HSA_LDFLAGS="$HSA_RUNTIME_LDFLAGS"
>> + PLUGIN_HSA_LIBS="-ldl"
>
> So this switched from directly linking against 'libhsa-runtime64.so' to a
> 'libdl'-based runtime linking variant.
>
> Previously, 'libhsa-runtime64.so' would've been found at run time via the
> standard search paths.
>
>> +if test "$HSA_RUNTIME_LIB" != ""; then
>> + HSA_RUNTIME_LIB="$HSA_RUNTIME_LIB/"
>> +fi
>> +
>> +AC_DEFINE_UNQUOTED([HSA_RUNTIME_LIB], ["$HSA_RUNTIME_LIB"],
>> + [Define path to HSA runtime.])
>
> That's new, to propagate '--with-hsa-runtime'/'--with-hsa-runtime-lib'
> into the HSA plugin source code.
>
>> --- a/libgomp/plugin/plugin-hsa.c
>> +++ b/libgomp/plugin/plugin-hsa.c
>
>> +static const char *hsa_runtime_lib;
>
>> static void
>> init_enviroment_variables (void)
>> {
>
>> + hsa_runtime_lib = secure_getenv ("HSA_RUNTIME_LIB");
>
> Unless overridden via the 'HSA_RUNTIME_LIB' environment variable...
>
>> + if (hsa_runtime_lib == NULL)
>> + hsa_runtime_lib = HSA_RUNTIME_LIB "libhsa-runtime64.so";
>
> ... we now default to '[HSA_RUNTIME_LIB]/libhsa-runtime64.so' (note
> 'HSA_RUNTIME_LIB' prefix!)...
>
>> +static bool
>> +init_hsa_runtime_functions (void)
>> +{
>> + void *handle = dlopen (hsa_runtime_lib, RTLD_LAZY);
>
> ..., which is then 'dlopen'ed here.
>
> That means, contrary to before, the GCC configure-time
> '--with-hsa-runtime' (by definition only valid for GCC configure/build as
> well as build-tree testing) leaks into the installed HSA libgomp plugin.
> That's a problem if your GCC build system (and build-tree testing)
> requires '--with-hsa-runtime' to specify a non-standard location (not in
> default search paths) but that location is not valid on your GCC
> deployment system (but it has leaked into the HSA libgomp plugin),
> meaning that (unless overridden via the 'HSA_RUNTIME_LIB' environment
> variable) 'libhsa-runtime64.so' is now no longer found via the standard
> search paths, because of the 'HSA_RUNTIME_LIB' prefix passed into
> 'dlopen'.
>
> Per my understanding this cannot be intentional, so I suggest to restore
> the previous behavior as per the attached "libgomp HSA/GCN plugins:
> don't
I honestly do not remember, it is quote possible. I'm not quite sure
what you mean by "previous behavior" (the previous behavior was static
linking, no?) though.
> prepend the 'HSA_RUNTIME_LIB' path to 'libhsa-runtime64.so'". OK to push
> such changes? I was tempted to push "as obvious", but maybe I fail to
> see the rationale behind this change?
>
> For avoidance of doubt, this change doesn't affect (build-tree) testsuite
> usage, where we have:
>
> libgomp/testsuite/libgomp-test-support.exp.in:set hsa_runtime_lib
> "@HSA_RUNTIME_LIB@"
>
> libgomp/testsuite/lib/libgomp.exp: append always_ld_library_path
> ":$hsa_runtime_lib"
>
> And, another data point:
>
> gcc/config/gcn/gcn-run.c:#define HSA_RUNTIME_LIB "libhsa-runtime64.so.1"
> [...]
> gcc/config/gcn/gcn-run.c: void *handle = dlopen (HSA_RUNTIME_LIB,
> RTLD_LAZY);
>
> Here, 'libhsa-runtime64.so.1' is 'dlopen'ed without prefix, and thus
> found via the standard search paths (as expected).
>
Right. From what I can tell at the moment, which is not much, the idea
was to be able to load it even from a non-standard path and specify that
path at configure time. If people think that is not useful and is
actually harmful, I guess it can go.
Martin
