235610 2022-01-25 14:39:02 -0800 Build failure with GCC 12: MediaTime.h:167:71: error: call to non-'constexpr' function 'WTF::MediaTime& WTF::MediaTime::operator=(const WTF::MediaTime&)' 2022-01-26 18:54:29 -0800 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build PC Linux RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=205723 https://bugs.webkit.org/show_bug.cgi?id=235501 P2 Normal --- 1 mcatanzaro mcatanzaro aboya benjamin bugs-noreply cdumez cmarcelo darin eric.carlson ews-watchlist glenn jer.noble mcatanzaro mgorse philipj sergio ysuzuki oldest_to_newest 1833868 0 mcatanzaro 2022-01-25 14:39:02 -0800 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. 1833875 1 449970 mcatanzaro 2022-01-25 15:12:35 -0800 Created attachment 449970 Patch 1833876 2 mcatanzaro 2022-01-25 15:13:12 -0800 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. 1833882 3 449970 darin 2022-01-25 15:19:56 -0800 Comment on attachment 449970 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 1833932 4 mcatanzaro 2022-01-25 16:59:30 -0800 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. 1833933 5 darin 2022-01-25 17:00:40 -0800 You can land first, I am willing to roll your change back out in a new version of my patch. 1833934 6 darin 2022-01-25 17:01:37 -0800 Wait, maybe you can just delete the custom assignment operator to fix this? Does it do anything valuable? 1833935 7 darin 2022-01-25 17:01:49 -0800 I think it doesn’t. 1833939 8 mcatanzaro 2022-01-25 17:04:22 -0800 (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. 1834170 9 mcatanzaro 2022-01-26 11:00:26 -0800 (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(); | ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1834191 10 ysuzuki 2022-01-26 11:28:31 -0800 (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). 1834216 11 mcatanzaro 2022-01-26 11:50:36 -0800 I'll try that. 1834243 12 mcatanzaro 2022-01-26 12:55:04 -0800 (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. 1834257 13 ysuzuki 2022-01-26 13:27:19 -0800 (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? 1834272 14 mcatanzaro 2022-01-26 13:46:47 -0800 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 1834376 15 ysuzuki 2022-01-26 17:35:50 -0800 (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 :) 1834377 16 mcatanzaro 2022-01-26 18:17:53 -0800 (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. 1834380 17 ews-feeder 2022-01-26 18:54:23 -0800 Committed r288660 (246468@main): <https://commits.webkit.org/246468@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449970. 449970 2022-01-25 15:12:35 -0800 2022-01-26 18:54:26 -0800 Patch bug-235610-20220125171234.patch text/plain 1807 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjg4NDQ4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDRlZWVkOGYxMWQ3MTk3NjJmZTg3OTM2 YzNkOTFhYTQyZmUyMDIxMWYuLmE4MzNkNTFhMzI3ZDQ0Njg1NmIyMDE1MzJlN2QzNTA4ZTViNmVj OTEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMjItMDEtMjUgIE1pY2hhZWwgQ2F0YW56YXJvICA8 bWNhdGFuemFyb0Bnbm9tZS5vcmc+CisKKyAgICAgICAgQnVpbGQgZmFpbHVyZSB3aXRoIEdDQyAx MjogTWVkaWFUaW1lLmg6MTY3OjcxOiBlcnJvcjogY2FsbCB0byBub24tJ2NvbnN0ZXhwcicgZnVu Y3Rpb24gJ1dURjo6TWVkaWFUaW1lJiBXVEY6Ok1lZGlhVGltZTo6b3BlcmF0b3I9KGNvbnN0IFdU Rjo6TWVkaWFUaW1lJiknCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0yMzU2MTAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICBUaGUgcHJvYmxlbSBpcyB0aGUgY29uc3RleHByIGNvbnN0cnVjdG9yIGNhbGxzIHRo ZSBub24tY29uc3RleHByIGNvcHkgYXNzaWdubWVudCBvcGVyYXRvci4KKyAgICAgICAgVGhpcyBp cyBlYXN5IHRvIGZpeCwgYXQgdGhlIGV4cGVuc2Ugb2YgcmVhZGFiaWxpdHksIGJ5IG9wZW4tY29k aW5nIHRoZSBkZXNpcmVkIHZhbHVlcyBoZXJlLgorCisgICAgICAgICogd3RmL01lZGlhVGltZS5o OgorICAgICAgICAoV1RGOjpNZWRpYVRpbWU6Ok1lZGlhVGltZSk6CisKIDIwMjItMDEtMjEgIFl1 c3VrZSBTdXp1a2kgIDx5c3V6dWtpQGFwcGxlLmNvbT4KIAogICAgICAgICBbSlNDXSBSZWxheCBE YXRlLnBhcnNlIHJlcXVpcmVtZW50CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL3d0Zi9NZWRpYVRp bWUuaCBiL1NvdXJjZS9XVEYvd3RmL01lZGlhVGltZS5oCmluZGV4IDcxYWNmMzI5ZWIzOTg5MzM1 MTJmZDdmM2Y1OGQ2MGJlY2ZmMjNjNTAuLmIwMTBiZDA2NDgzYWE3ZDQ1NWVmNWNlM2UzMGUxODhi MTA2YTdkNzggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvd3RmL01lZGlhVGltZS5oCisrKyBiL1Nv dXJjZS9XVEYvd3RmL01lZGlhVGltZS5oCkBAIC0xNjQsNyArMTY0LDE3IEBAIGNvbnN0ZXhwciBN ZWRpYVRpbWU6Ok1lZGlhVGltZShpbnQ2NF90IHZhbHVlLCB1aW50MzJfdCBzY2FsZSwgdWludDhf dCBmbGFncykKICAgICBpZiAoc2NhbGUgfHwgIShmbGFncyAmIFZhbGlkKSkKICAgICAgICAgcmV0 dXJuOwogCi0gICAgKnRoaXMgPSB2YWx1ZSA8IDAgPyBuZWdhdGl2ZUluZmluaXRlVGltZSgpIDog cG9zaXRpdmVJbmZpbml0ZVRpbWUoKTsKKyAgICBpZiAodmFsdWUgPCAwKSB7CisgICAgICAgIC8v IE5lZ2F0aXZlIGluZmluaXRlIHRpbWUKKyAgICAgICAgbV90aW1lVmFsdWUgPSAtMTsKKyAgICAg ICAgbV90aW1lU2NhbGUgPSAxOworICAgICAgICBtX3RpbWVGbGFncyA9IE5lZ2F0aXZlSW5maW5p dGUgfCBWYWxpZDsKKyAgICB9IGVsc2UgeworICAgICAgICAvLyBQb3NpdGl2ZSBpbmZpbml0ZSB0 aW1lCisgICAgICAgIG1fdGltZVZhbHVlID0gMDsKKyAgICAgICAgbV90aW1lU2NhbGUgPSAxOwor ICAgICAgICBtX3RpbWVGbGFncyA9IFBvc2l0aXZlSW5maW5pdGUgfCBWYWxpZDsKKyAgICB9CiB9 CiAKIGlubGluZSBNZWRpYVRpbWUgb3BlcmF0b3IqKGludDMyX3QgbGhzLCBjb25zdCBNZWRpYVRp bWUmIHJocykgeyByZXR1cm4gcmhzLm9wZXJhdG9yKihsaHMpOyB9Cg==