RESOLVED FIXED 235610
Build failure with GCC 12: MediaTime.h:167:71: error: call to non-'constexpr' function 'WTF::MediaTime& WTF::MediaTime::operator=(const WTF::MediaTime&)'
https://bugs.webkit.org/show_bug.cgi?id=235610
Summary Build failure with GCC 12: MediaTime.h:167:71: error: call to non-'constexpr'...
Michael Catanzaro
Reported 2022-01-25 14:39:02 PST
WebKitGTK 2.35.1 fails to build with GCC 12 on 32-bit ARM (and not other architectures): In file included from /builddir/build/BUILD/webkitgtk-2.35.1/Source/WTF/wtf/MediaTime.cpp:30: /builddir/build/BUILD/webkitgtk-2.35.1/Source/WTF/wtf/MediaTime.h: In constructor 'constexpr WTF::MediaTime::MediaTime(int64_t, uint32_t, uint8_t)': /builddir/build/BUILD/webkitgtk-2.35.1/Source/WTF/wtf/MediaTime.h:167:71: error: call to non-'constexpr' function 'WTF::MediaTime& WTF::MediaTime::operator=(const WTF::MediaTime&)' 167 | *this = value < 0 ? negativeInfiniteTime() : positiveInfiniteTime(); | ^ /builddir/build/BUILD/webkitgtk-2.35.1/Source/WTF/wtf/MediaTime.h:68:16: note: 'WTF::MediaTime& WTF::MediaTime::operator=(const WTF::MediaTime&)' declared here 68 | MediaTime& operator=(const MediaTime& rhs); | ^~~~~~~~ I'm not sure what makes 32-bit ARM special here. Anyway the problem is clear enough: the constexpr constructor calls the non-constexpr assignment operator. I will test a potential fix.
Attachments
Patch (1.76 KB, patch)
2022-01-25 15:12 PST, Michael Catanzaro
no flags
Michael Catanzaro
Comment 1 2022-01-25 15:12:35 PST
Michael Catanzaro
Comment 2 2022-01-25 15:13:12 PST
This patch at least doesn't break the build with GCC 11 on x86_64. My GCC 12 armv7hl build to ensure it actually fixes the bug is WIP.
Darin Adler
Comment 3 2022-01-25 15:19:56 PST
Comment on attachment 449970 [details] Patch This will not be needed once I land the patch in bug 235501, which eliminates this bad pattern (using operator= to implement a constructor), but it’s OK to land it now, I guess. Unless you are wiling to wait
Michael Catanzaro
Comment 4 2022-01-25 16:59:30 PST
Hmm, that's a big patch. Normally, I would say "let's land the small patch first, then rebase your big patch on it" because my changes are small and it should be easy to adjust your patch for mine after it lands. However, looking at that diff, I notice you actually don't touch the constexpr constructor at all: you just removed the copy assignment operator, to prefer use of the default copy assignment operator. That seems good, but it means my change won't generate a merge conflict with yours, and we don't actually want my change anymore after yours lands. So you would just have to remember to revert my patch in yours, and that feels not worth it, at least if you're planning to land your patch soon. In short: it's probably best to not commit this. Let's instead wait for your patch.
Darin Adler
Comment 5 2022-01-25 17:00:40 PST
You can land first, I am willing to roll your change back out in a new version of my patch.
Darin Adler
Comment 6 2022-01-25 17:01:37 PST
Wait, maybe you can just delete the custom assignment operator to fix this? Does it do anything valuable?
Darin Adler
Comment 7 2022-01-25 17:01:49 PST
I think it doesn’t.
Michael Catanzaro
Comment 8 2022-01-25 17:04:22 PST
(In reply to Darin Adler from comment #6) > Wait, maybe you can just delete the custom assignment operator to fix this? > Does it do anything valuable? I think so. At least, that's what you did, so if it's not safe then I assume your patch has a problem. :P Will give that a try tomorrow.
Michael Catanzaro
Comment 9 2022-01-26 11:00:26 PST
(In reply to Darin Adler from comment #6) > Wait, maybe you can just delete the custom assignment operator to fix this? > Does it do anything valuable? This didn't work: /builddir/build/BUILD/webkitgtk-2.35.1/Source/WTF/wtf/MediaTime.h:164:71: warning: implicitly-declared 'constexpr WTF::MediaTime& WTF::MediaTime::operator=(const WTF::MediaTime&)' is deprecated [-Wdeprecated-copy] 164 | *this = value < 0 ? negativeInfiniteTime() : positiveInfiniteTime(); | ^ /builddir/build/BUILD/webkitgtk-2.35.1/Source/WTF/wtf/MediaTime.h:58:5: note: because 'WTF::MediaTime' has user-provided 'WTF::MediaTime::MediaTime(const WTF::MediaTime&)' 58 | MediaTime(const MediaTime& rhs); | ^~~~~~~~~ /builddir/build/BUILD/webkitgtk-2.35.1/Source/WTF/wtf/MediaTime.h:164:23: error: expression '((value < 0) ? (& WTF::MediaTime::negativeInfiniteTime()) : (& WTF::MediaTime::positiveInfiniteTime()))' is not a constant expression 164 | *this = value < 0 ? negativeInfiniteTime() : positiveInfiniteTime(); | ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Yusuke Suzuki
Comment 10 2022-01-26 11:28:31 PST
(In reply to Michael Catanzaro from comment #9) > (In reply to Darin Adler from comment #6) > > Wait, maybe you can just delete the custom assignment operator to fix this? > > Does it do anything valuable? > > This didn't work: > > /builddir/build/BUILD/webkitgtk-2.35.1/Source/WTF/wtf/MediaTime.h:164:71: > warning: implicitly-declared 'constexpr WTF::MediaTime& > WTF::MediaTime::operator=(const WTF::MediaTime&)' is deprecated > [-Wdeprecated-copy] > 164 | *this = value < 0 ? negativeInfiniteTime() : > positiveInfiniteTime(); > | > ^ > /builddir/build/BUILD/webkitgtk-2.35.1/Source/WTF/wtf/MediaTime.h:58:5: > note: because 'WTF::MediaTime' has user-provided > 'WTF::MediaTime::MediaTime(const WTF::MediaTime&)' > 58 | MediaTime(const MediaTime& rhs); > | ^~~~~~~~~ > /builddir/build/BUILD/webkitgtk-2.35.1/Source/WTF/wtf/MediaTime.h:164:23: > error: expression '((value < 0) ? (& WTF::MediaTime::negativeInfiniteTime()) > : (& WTF::MediaTime::positiveInfiniteTime()))' is not a constant expression > 164 | *this = value < 0 ? negativeInfiniteTime() : > positiveInfiniteTime(); > | > ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Let's remove both, `MediaTime::MediaTime(const MediaTime& rhs)` constructor, and `operator=(const MediaTime&)`. Both are the same to the default-generated ones. So we can remove them, and they become constexpr (since MediaTime fields can be copied with constexpr).
Michael Catanzaro
Comment 11 2022-01-26 11:50:36 PST
I'll try that.
Michael Catanzaro
Comment 12 2022-01-26 12:55:04 PST
(In reply to Yusuke Suzuki from comment #10) > Let's remove both, `MediaTime::MediaTime(const MediaTime& rhs)` constructor, > and `operator=(const MediaTime&)`. > Both are the same to the default-generated ones. So we can remove them, and > they become constexpr (since MediaTime fields can be copied with constexpr). Doesn't work. Same error message. I'm not so sure that the default copy assignment operator is actually constexpr.
Yusuke Suzuki
Comment 13 2022-01-26 13:27:19 PST
(In reply to Michael Catanzaro from comment #12) > (In reply to Yusuke Suzuki from comment #10) > > Let's remove both, `MediaTime::MediaTime(const MediaTime& rhs)` constructor, > > and `operator=(const MediaTime&)`. > > Both are the same to the default-generated ones. So we can remove them, and > > they become constexpr (since MediaTime fields can be copied with constexpr). > > Doesn't work. Same error message. I'm not so sure that the default copy > assignment operator is actually constexpr. Same error message sounds strange since the error message is pointing the user-provided MediaTime constructor, which should be removed now. Can you paste the error message?
Michael Catanzaro
Comment 14 2022-01-26 13:46:47 PST
Well it's not the same error message from comment #0, but it's the same as comment #9: In file included from /builddir/build/BUILD/webkitgtk-2.35.1/Source/WTF/wtf/MediaTime.cpp:30: /builddir/build/BUILD/webkitgtk-2.35.1/Source/WTF/wtf/MediaTime.h: In constructor 'constexpr WTF::MediaTime::MediaTime(int64_t, uint32_t, uint8_t)': /builddir/build/BUILD/webkitgtk-2.35.1/Source/WTF/wtf/MediaTime.h:165:23: error: expression '((value < 0) ? (& WTF::MediaTime::negativeInfiniteTime()) : (& WTF::MediaTime::positiveInfiniteTime()))' is not a constant expression 165 | *this = value < 0 ? negativeInfiniteTime() : positiveInfiniteTime(); | ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Build log: https://kojipkgs.fedoraproject.org//work/tasks/972/81960972/build.log
Yusuke Suzuki
Comment 15 2022-01-26 17:35:50 PST
(In reply to Michael Catanzaro from comment #14) > Well it's not the same error message from comment #0, but it's the same as > comment #9: > > In file included from > /builddir/build/BUILD/webkitgtk-2.35.1/Source/WTF/wtf/MediaTime.cpp:30: > /builddir/build/BUILD/webkitgtk-2.35.1/Source/WTF/wtf/MediaTime.h: In > constructor 'constexpr WTF::MediaTime::MediaTime(int64_t, uint32_t, > uint8_t)': > /builddir/build/BUILD/webkitgtk-2.35.1/Source/WTF/wtf/MediaTime.h:165:23: > error: expression '((value < 0) ? (& WTF::MediaTime::negativeInfiniteTime()) > : (& WTF::MediaTime::positiveInfiniteTime()))' is not a constant expression > 165 | *this = value < 0 ? negativeInfiniteTime() : > positiveInfiniteTime(); > | > ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Build log: > https://kojipkgs.fedoraproject.org//work/tasks/972/81960972/build.log OK, then, we should move negativeInfiniteTime / positiveInfiniteTime (in MediaTime.cpp) to MediaTime.h as a constexpr static function under MediaTime, and then it looks fine. static constexpr MediaTime positiveInfiniteTime() { return { 0, 1, PositiveInfinite | Valid }; } static constexpr MediaTime negativeInfiniteTime() { return { -1, 1, NegativeInfinite | Valid }; } But https://bugs.webkit.org/show_bug.cgi?id=235501 will fix these things more holistic way. So, for now, I think it is OK to land the current patch to avoid compile error :)
Michael Catanzaro
Comment 16 2022-01-26 18:17:53 PST
(In reply to Yusuke Suzuki from comment #15) > OK, then, we should move negativeInfiniteTime / positiveInfiniteTime (in > MediaTime.cpp) to MediaTime.h as a constexpr static function under > MediaTime, and then it looks fine. > > static constexpr MediaTime positiveInfiniteTime() { return { 0, 1, > PositiveInfinite | Valid }; } > static constexpr MediaTime negativeInfiniteTime() { return { -1, 1, > NegativeInfinite | Valid }; } Oh, hmm, you might be right. > But https://bugs.webkit.org/show_bug.cgi?id=235501 will fix these things > more holistic way. > So, for now, I think it is OK to land the current patch to avoid compile > error :) Ack. I'm sure Darin will remember to revert it as part of bug #235501.
EWS
Comment 17 2022-01-26 18:54:23 PST
Committed r288660 (246468@main): <https://commits.webkit.org/246468@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449970 [details].
Note You need to log in before you can comment on or make changes to this bug.