Bug 235610 – Build failure with GCC 12: MediaTime.h:167:71: error: call to non-'constexpr' function 'WTF::MediaTime& WTF::MediaTime::operator=(const WTF::MediaTime&)'

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 449970 [details]
Patch

[Michael Catanzaro]

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 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]

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]

You can land first, I am willing to roll your change back out in a new version of my patch.

[Darin Adler]

Wait, maybe you can just delete the custom assignment operator to fix this? Does it do anything valuable?

[Darin Adler]

I think it doesn’t.

[Michael Catanzaro]

(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]

(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]

(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]

I'll try that.

[Michael Catanzaro]

(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]

(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]

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]

(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]

(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]

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].