compilers/pgi: generate header dependency args for NVHPC/PGI - #16161
compilers/pgi: generate header dependency args for NVHPC/PGI#16161stoiver wants to merge 1 commit into
Conversation
|
Unreadable LLM output. Remove the test and the useless comments and there will be a chance of the patch being reviewed. |
|
Example Why would I care about past wrong behavior? The place for that is the commit messages, not comments. |
Please do it then. The change is not awful—it's obviously useful and doesn't have blatant algorithmic issues—so it's a good starting point. What's wrong is that you're asking others to complete the work for you when it wasn't even you who did the work in the first place, since you had a machine doing it for you. Meson is run by volunteers. Asking for anything more than this cursory check will require you to put in some effort: make our job easier as much as the LLM made it easier for you. |
PGICompiler did not implement get_dependency_gen_args(), so it inherited the base class stub returning []. Meson still emitted 'deps = gcc' and a depfile into the ninja rule, but the compile line never received -MD/-MF, so no depfile was written: ninja recorded zero dependencies for every object and a changed header never triggered a rebuild. Incremental builds could then silently produce binaries whose translation units disagreed about struct layout. Same defect as mesonbuild#11969 (CUDA), fixed in mesonbuild#12665, in a different compiler class. C and C++ only. nvfortran accepts -MD/-MT/-MF and exits 0 but writes no depfile; NVHPC's help describes -M as 'Print dependencies to stdout in C++'. Putting the override on the shared PGICompiler mixin would therefore have left Fortran builds emitting the flags with ninja still waiting for a depfile that never arrives. Hence a separate PGIDependencyMixin, used by the four C/C++ classes and not by the Fortran ones. -MT rather than -MQ: nvc accepts -MQ but ignores its argument and writes the literal string 'mttarget' as the depfile target, in both the -MQ x and -MQ=x forms. Since -MT does not escape the target for Make, this also needs the escaping added for CUDA in mesonbuild#12665, which was gated on langname == 'cuda'. Rather than add a second special case, Compiler.needs_escaped_depfile_target() now selects it, returning True for CUDA and for PGI C/C++. The ninja variable CUDA_ESCAPED_TARGET is renamed ESCAPED_DEPFILE_TARGET as it is no longer CUDA-specific. No behaviour change for CUDA. Tested with nvc, nvc++ and nvfortran 26.3, and on an NVHPC project of 34 objects: all 34 gain recorded dependencies where they previously had none, and touching a shared header rebuilds the 13 objects that include it. Fixes mesonbuild#16160
29c1e71 to
8a245e5
Compare
|
Thanks — the Fortran question was the right one, and it found a bug.
v2 puts it on a separate Tests removed, and the comments cut to what a future reader needs — why Tested on nvc/nvc++/nvfortran 26.3 and on a real NVHPC project: all 34 objects The one red CI job, Ubuntu Rolling (clang, clang++), fails identically on master On your wider point — fair. The patch was AI-assisted. We should have answered the |
Fixes #16160.
PGICompilerdid not implementget_dependency_gen_args(), so it inherited the baseclass stub returning
[]. Meson still emitteddeps = gccand adepfileinto the ninjarule, but the compile line never received
-MD/-MF, so no depfile was ever written:ninja recorded zero dependencies for every object and a changed header never triggered
a rebuild.
Same defect as #11969 (CUDA), fixed in #12665, in a different compiler class.
This is not just a stale-build annoyance. An incremental build can silently produce a
binary whose translation units disagree about struct layout — no compile error, no
warning. In the project where I hit this, a member in a shared header shifted, only some
objects were rebuilt, and two struct members aliased: the program printed a nonsense value
and hung.
-MQmust not be used herenvcaccepts-MQbut ignores its argument, writing the literal stringmttargetasthe depfile target, which ninja then silently discards. That failure looks identical to
having no fix at all, so it is worth stating explicitly. Verified on nvc 26.3:
So
-MTis used, as for nvcc.Escaping
-MTdoes not Make-escape the target, so this needs the escaping #12665 added for CUDA.That was gated on
langname == 'cuda'/compiler.get_language() == 'cuda'. Rather thanadd a second special case, this adds a compiler predicate:
returning
Truefor CUDA and PGI, with bothninjabackend.pygates switched to it. Theninja variable
CUDA_ESCAPED_TARGETis renamedESCAPED_DEPFILE_TARGETsince it is nolonger CUDA-specific. No behaviour change for CUDA.
Tests
Two tests in
unittests/internaltests.py:test_pgi_dependency_gen_args— the flags themselves.test_depfile_target_escaping_is_declared— tiesget_dependency_gen_argstoneeds_escaped_depfile_targetas an invariant across GCC, Clang and NVHPC: emitting-MTrequiresTrue, emitting-MQrequiresFalse. This is the failure a futurecompiler addition is most likely to hit, and it only shows up for object paths
containing a space,
$or#.Both fail on unpatched master (
[] != ['-MD', '-MT', 'foo.o', '-MF', 'foo.o.d']) and passwith the change. Full
internaltests.py: 78 passed, 129 subtests.Real-world verification
On an NVHPC project (~34 objects, C plus Cython-generated C,
-mp=gpu):The 13 are exactly the objects including that header, across both extension modules. That
project's own test suites pass unchanged.
Open questions
>= 10.2. I only have nvc 26.3 and do not knowthe earliest NVHPC/PGI release supporting
-MD/-MT. Happy to add a bound if someonewith older toolchains can name one.
PGICompilerbacks bothPGICCompiler(pgcc) and theNvidiaHPC_*classes. I verified NVHPC only. If legacypgccdiffers, the overridemay belong on the NVHPC classes instead of the shared mixin.