Description of problem: Since binutils-2.35.1-5.fc34.x86_64 libvirt build has started failing with missing symbols: https://kojipkgs.fedoraproject.org//work/tasks/494/53840494/build.log /usr/bin/ld: tests/libtest_qemu_driver.so: undefined reference to `__open_missing_mode' AFAICT, libvirt passes "mode" in all cases where we pass O_CREAT for flags. Disabling LTO removes the error, as does downgrading to binutils-2.35.1-4.fc34.x86_64 Unfortunately I can't determine a simple standalone reproducer yet. Version-Release number of selected component (if applicable): binutils-2.35.1-5.fc34.x86_64 How reproducible: Always Steps to Reproduce: 1. Scratch build of current libvirt source against rawhide. 2. 3. Actual results: /usr/bin/ld: tests/libtest_qemu_driver.so: undefined reference to `__open_missing_mode' Expected results: Compiles succesfully :-) Additional info:
The same problem occurs when building the nodejs package. I do not know the cause yet, but I am investigating.
Created attachment 1723757 [details] Proposed patch Fix calls to open() that need a third argument.
Hi Daniel, Despite the very unhelpful linker error message. I think that this problems is actually due to a bug in the qemu sources that form part of libvirt. Specifically in at line 6237 there is: if ((ctxt->writefd = open(ctxt->path, O_WRONLY | O_CREAT | O_APPEND, S_IRUSR | S_IWUSR)) < 0) { This call to open() is missing a mode argument for the O_CREAT flag. I believe that it is this the root cause of the problem. (Actually there appears to be a similar omission on line 6405 of that file as well...) What I think is happening is that without LTO, the erroneous code is being eliminated by the compiler, and so no error has been seen before. Now that LTO is enabled by default (in rawhide) the erroneous code is being retained so that it is available for the final LTO compilation. Before the 2.35.1-5.fc34 binutils release however a bug in the linker's handling of lto-plugin symbols meant that the code was never pulled into the lto-compilation-then-link process, and so the issue was not seen. (The linker bug meant that sometimes libraries would not be pulled into a link, if an earlier library appeared to provide all of the needed symbols). With 2.35.1-5.fc34 the linker bug was fixed and now the lto-compilation process sees the code. I theorize however that definition of the open() function (in /usr/include/bits/fcntl2.h) had somehow lost error function attribute that is supposed to be on the __open_missing_mode symbol, (presumably because of an LTO bug) and so it now compiles a call to the non-existant __open_missing_mode function... Anyway, I am going to upload a potential patch for libvirt. Although my lack of familiarity with the package means that I do not know how to apply it. Hopefully you will have more success with testing it. Cheers Nick
Sorry, this patch is not correct. Both calls already contain a mode argument. I think this is a GCC bug. If there is a reference left to __open_missing_mode, the compilation *must* fail: 'error ("MESSAGE")' 'warning ("MESSAGE")' If the 'error' or 'warning' attribute is used on a function declaration and a call to such a function is not eliminated through dead code elimination or other optimizations, an error or warning (respectively) that includes MESSAGE is diagnosed. This is useful for compile-time checking, especially together with '__builtin_constant_p' and inline functions where checking the inline function arguments is not possible through 'extern char [(condition) ? 1 : -1];' tricks.
(In reply to Nick Clifton from comment #3) > Hi Daniel, > > Despite the very unhelpful linker error message. I think that this > problems is actually due to a bug in the qemu sources that form part of > libvirt. Specifically in > > at line 6237 there is: > > if ((ctxt->writefd = open(ctxt->path, O_WRONLY | O_CREAT | O_APPEND, > S_IRUSR | S_IWUSR)) < 0) { > > This call to open() is missing a mode argument for the O_CREAT flag. I > believe that it is this the root cause of the problem. Am I misunderstanding something? I can see three arguments passed and the last one (mode) is "S_IRUSR | S_IWUSR" which is a verbose way of writing 0600.
Do any functions calling open use something like optimize (0) attribute or similar ways to prevent optimizations? Because the glibc header only enables these open compile time checks if using _FORTIFY_SOURCE{,=2} and if -O1 or higher; furthermore, the function is extern inline __attribute__((always_inline)), so it should be inlined already by the early inliner (unless -fno-early-inlining ?), and __va_arg_pack_len () < 1 should fold into 1 < 1 when the mode is specified. Thus, unless optimizations are massively disabled, I don't see how that __open_missing_mode could survive until IPA (and thus it shouldn't be even LTO related). So, if the source has optimize (0) etc., avoid calling open/open64/etc. in those functions or drop the attribute or call them from helper functions that are optimized and may not be inlined. Otherwise, it would be interesting to see -fdump-tree-all dumps from a TU which results in the __open_missing_mode calls in the IL. You can just run nm on the *.o file and see which one it is.
By trial and error I've converged to a minimal set of CFLAGS="-O2 -flto=auto" which gets this error: cc -o tests/cputest src/libvirt_probes.o tests/cputest.p/cputest.c.o -Wl,--as-needed -Wl,--no-undefined -Wl,-export-dynamic -pie -Wl,--whole-archive -Wl,--start-group tests/libtest_utils.a tests/libtest_utils_qemu.a -Wl,--no-whole-archive -O2 -flto=auto src/libvirt.so.0.6009.0 tests/libtest_utils_qemu_monitor.a tests/libtest_qemu_driver.so -Wl,--no-copy-dt-needed-entries -Wl,-export-dynamic -ldl /usr/lib64/libglib-2.0.so /usr/lib64/libgobject-2.0.so /usr/lib64/libgio-2.0.so /usr/lib64/libgnutls.so /usr/lib64/libnl-3.so /usr/lib64/libnl-route-3.so /usr/lib64/libxml2.so /usr/lib64/libsasl2.so -lselinux /usr/lib64/libtirpc.so /usr/lib64/libyajl.so -Wl,-export-dynamic -lselinux -Wl,-export-dynamic -lselinux -Wl,-export-dynamic -lselinux -Wl,--end-group '-Wl,-rpath,$ORIGIN/../src:$ORIGIN/' -Wl,-rpath-link,/home/zippy/tmp/libvirt.git/_build/src -Wl,-rpath-link,/home/zippy/tmp/libvirt.git/_build/tests /usr/bin/ld: tests/libtest_qemu_driver.so: undefined reference to `__open_missing_mode' collect2: error: ld returned 1 exit status But if I disable optimizations CFLAGS="-O0 -flto=auto" then I can't reproduce and the build succeeds. Hope this helps.
So, can you please nm tests/cputest.p/cputest.c.o tests/libtest_utils.a tests/libtest_utils_qemu.a src/libvirt.so.0.6009.0 tests/libtest_utils_qemu_monitor.a | less and find there which *.o file has __open_missing_mode references?
That did no yield any, but using a brute search I've found the following: $ pwd /home/zippy/tmp/libvirt.git/_build $ for i in $(find . -name "*.o"); do if test -n "$(nm $i | grep __open_missing_mode)" ; then echo $i; fi; done ./src/util/libvirt_util.a.p/virfdstream.c.o ./src/qemu/libvirt_driver_qemu_impl.a.p/qemu_domain.c.o In virfdstream.c there are only two open(): https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virfdstream.c#L1267 if (oflags & O_CREAT) fd = open(path, oflags, mode); else fd = open(path, oflags); but in qemu_domain.c there is one open that looks very suspicious: https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_domain.c#L6254 if ((ctxt->readfd = open(ctxt->path, O_RDONLY, S_IRUSR | S_IWUSR)) < 0) { Nevertheless, compiler did not complain, and in fact, when I remove the mode, it doesn't fix the problem. I've inspected other calls of open() in both files and found nothing suspicious.
So, can you please recompile virfdstream.c with the options that were used to compile it with additional -save-temps and attach here virfdstream.i it creates + paste the options? Thanks.
Note if (oflags & O_CREAT) fd = open(path, oflags, mode); else fd = open(path, oflags); doesn't make any sense unless mode would be e.g. uninitialized if oflags doesn't have O_CREAT set (but that is not the case). Just do fd = open(path, oflags, mode); Anyway, I'd really like to understand what's going on.
Created attachment 1723813 [details] virfdstream.i
(In reply to Jakub Jelinek from comment #11) > This got me experimenting, and if I provide mode for all open() (in those two files virfdstream.c and qemu_domain.c) - even those that open only for reading then the problem is gone. But this is not that surpising, is it?
You certainly don't need to provide a mode to all open calls. All I wanted to say is that the above pass mode only if O_CREAT is in the flags and not otherwise is a complete optimization anti-pattern, it will make the code slower unless oflags is optimized into a constant. Anyway, in the tree dumps I see that before IPA in the one function which isn't inline yet we have basically: oflags |= O_NOCCTY; if (oflags & O_CREAT) fd = __builtin_constant_p (oflags) ? __open_alias (path, oflags, mode) : __open_alias (path, oflags, mode); else if (__builtin_constant_p (oflags)) { if ((oflags & __O_TMPFILE) == __O_TMPFILE) { __open_missing_mode (); fd = __open_2 (path, oflags); } else fd = __open_alias (path, oflags); } else fd = __open_2 (path, oflags); and that function is pretty large, called from 4 places where two of them pass in their parameter | O_CREAT. The reason the O_CREAT check inside is optimized away is because of the O_CREAT check from the user code. Now, without -flto both of the __builtin_constant_p (oflags) calls are folded into 0 during vrp1 and the __open_missing_mode () call is optimized away because of that during cfg cleanup at the end of vrp. So something else must be going on during LTO, perhaps some callers pass constant to oflags (otherwise I don't see how it would fold to non-zero or not be optimized away), but for some reason the oflags & __O_TMPFILE check isn't optimized to 0 or something. That said, unless some caller calls these functions with O_TMPFILE, then the error would be completely valid and it would be libvirt bug. Thus, what oflags pass callers of virFDStreamOpenFile or virFDStreamOpenBlockDevice to those functions (or their callers etc.)?
(In reply to Jakub Jelinek from comment #14) > You certainly don't need to provide a mode to all open calls. All I wanted > to say is that the above pass mode only if O_CREAT is in the flags and not > otherwise is a complete optimization anti-pattern, it will make the code > slower unless oflags is optimized into a constant. I know that I don't need to provide mode. I'm just experimenting and seeing what sticks on the wall. > Anyway, in the tree dumps I see that before IPA in the one function which > isn't inline yet we have basically: > oflags |= O_NOCCTY; > if (oflags & O_CREAT) > fd = __builtin_constant_p (oflags) ? __open_alias (path, oflags, mode) : > __open_alias (path, oflags, mode); > else if (__builtin_constant_p (oflags)) > { > if ((oflags & __O_TMPFILE) == __O_TMPFILE) > { > __open_missing_mode (); > fd = __open_2 (path, oflags); > } > else > fd = __open_alias (path, oflags); > } > else > fd = __open_2 (path, oflags); > and that function is pretty large, called from 4 places where two of them > pass in their parameter | O_CREAT. > The reason the O_CREAT check inside is optimized away is because of the > O_CREAT check from the user code. > Now, without -flto both of the __builtin_constant_p (oflags) calls are > folded into 0 during vrp1 and the __open_missing_mode () call is optimized > away because of that during cfg cleanup at the end of vrp. > So something else must be going on during LTO, perhaps some callers pass > constant to oflags (otherwise I don't see how it would fold to non-zero or > not be optimized away), but for some reason the oflags & __O_TMPFILE check > isn't optimized to 0 or something. > That said, unless some caller calls these functions with O_TMPFILE, then the > error would be completely valid and it would be libvirt bug. > > Thus, what oflags pass callers of virFDStreamOpenFile or > virFDStreamOpenBlockDevice to those functions (or their callers etc.)? Nothing passes O_TMPFILE, the only occurrence within the whole code base of that flag is in one comment. And callers of virFDStreamOpenFile() and virFDStreamOpenBlockDevice() pass constant oflags: src/lxc/lxc_driver.c:2781: if (virFDStreamOpenFile(st, chr->source->data.file.path, src/lxc/lxc_driver.c-2782- 0, 0, O_RDWR) < 0) src/qemu/qemu_driver.c:3485: if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY) < 0) src/test/test_driver.c:7749: if (virFDStreamOpenFile(st, PKGDATADIR "/test-screenshot.png", 0, 0, O_RDONLY) < 0) src/vbox/vbox_common.c:7423: if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY) < 0) { tests/fdstreamtest.c:82: if (virFDStreamOpenFile(st, file, tests/fdstreamtest.c-83- PATTERN_LEN / 2, PATTERN_LEN * 9, tests/fdstreamtest.c-84- O_RDONLY) < 0) src/storage/storage_util.c:2439: return virFDStreamOpenBlockDevice(stream, target_path, src/storage/storage_util.c-2440- offset, len, sparse, O_WRONLY); src/storage/storage_util.c:2470: return virFDStreamOpenBlockDevice(stream, target_path, src/storage/storage_util.c-2471- offset, len, sparse, O_RDONLY);
I ran into this problem with binutils-2.35-11.fc33.s390x on Fedora 33. Is this is also happening on x86?
Created attachment 1724523 [details] rhbz-1889763-zziplib.c Legacy code of zziplib, which takes an address of &open, also hit this issue. A minimal example is attached.
Created attachment 1724524 [details] rhbz-1889763-zziplib.c
$ curl -JO 'https://bugzilla.redhat.com/attachment.cgi?id=1724524' $ sh rhbz-1889763-zziplib.c + gcc rhbz-1889763-zziplib.c -Wp,-D_FORTIFY_SOURCE=2 -Wl,--export-dynamic -O2 -flto=auto -shared -o rhbz-1889763-zziplib.c.so + readelf -s rhbz-1889763-zziplib.c.so + grep __open 3: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND __open_too_many_args 5: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND __open_missing_mode 42: 0000000000000000 0 FUNC GLOBAL DEFAULT UND __open_2@@GLIBC_2.7 + exit 0
I can't reproduce using your above minimal example, one gets there open and __open_2 references which is ok, no __open_missing_mode. And, while I can reproduce it with libvirt, it doesn't seem to look like a GCC bug, but more likely binutils (or perhaps the linker plugin). Because, if I look at the assembly coming out of lto1 which is what all the *.o files with LTO bytecode are supposed to be replaced with, there is no __open_missing_mode at all. Sure, in the *.o files when using gcc-nm or when nm can find the linker plugin __open_missing_mode is seen, but not without it.
Strange, now I can using the #c18 testcase, but those __open_too_many_args and __open_missing_mode strangely only show up in .dynsym and not in .symtab, that is the first time I've seen symbols only in the former and not in the latter. So it really seems like a binutils bug.
Another example where binutils 2.35.1 behaves differently from 2.31: #include <unistd.h> int foo (int x) { if (__builtin_constant_p (x)) return getpid (); return 0; } gcc -shared -fpic -O2 -flto=auto -Wl,-E -o test.so test.c With binutils 2.35.1 undefined getpid symbol shows up in both .dynsym and .symtab, with 2.31 it doesn't. And, if one looks at objdump -dr test.so , getpid isn't called there at all, and it doesn't show up in test*ltrans*.s either (when -save-temps is added).
I *think* that I may have found the problem. Please could you try binutils-2.35.1-9.fc34 and see if this resolves the issue.
Yep, binutils-2.35.1-9.fc34 fixes the problem for me.
(In reply to Nick Clifton from comment #23) > I *think* that I may have found the problem. Please could you try > binutils-2.35.1-9.fc34 and see if this resolves the issue. It fixes the zziplib build issue as well. Thanks for the fix!
Ah no, sorry - the patch does not work. It basically reverts the original change that triggered this bug, but it does not solve the bug that that change was trying to fix. H.J.Lu has posted an alternative patch, so I am trying that out now.
Right - H.J.'s patch works, so I have brought it in to rawhide. Please try binutils-2.35.1-10.fc34.
We have a user reporting the same libvirt error message about _open_missing_mode when on F33 too now. Does the fix need to be applied to F33 too ?
I confirm that both attachment #1724524 [details] and the build of zziplib-0.13.69-9.fc34 work as expected with binutils-2.35.1-10.fc34.
(In reply to Daniel Berrangé from comment #28) > We have a user reporting the same libvirt error message about > _open_missing_mode when on F33 too now. Does the fix need to be applied to > F33 too ? Yes it does. Working on it now.
FEDORA-2020-6f4c3e05ea has been submitted as an update to Fedora 33. https://bodhi.fedoraproject.org/updates/FEDORA-2020-6f4c3e05ea
FEDORA-2020-6f4c3e05ea has been pushed to the Fedora 33 testing repository. In short time you'll be able to install the update with the following command: `sudo dnf upgrade --enablerepo=updates-testing --advisory=FEDORA-2020-6f4c3e05ea` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2020-6f4c3e05ea See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2020-6f4c3e05ea has been pushed to the Fedora 33 stable repository. If problem still persists, please make note of it in this bug report.