List items

Items from the current list are shown below.


1 Sep 2023 : Day 16 #
Yesterday we fixed a problem causing the MOC to fail building, as well as refactoring some functions that had been renamed upstream. This morning I was happy to see that the build moved forwards beyond those errors. So today we're on to something new. Here's what we're dealing with:
153:38.02 In file included from Unified_cpp_js_src_jit14.cpp:47:
153:38.02 ${PROJECT}/gecko-dev/js/src/jit/arm64/vixl/MozCpu-vixl.cpp:
          In function ‘int membarrier( ’ was not declared in this scope
153:38.02      return syscall(__NR_membarrier, cmd, flags);
153:38.02                     ^~~~~~~~~~~~~~~
153:38.03 ${PROJECT}/gecko-dev/js/src/jit/arm64/vixl/MozCpu-vixl.cpp:52:20: note:
          suggested alternative: ‘membarrier’
153:38.03      return syscall(__NR_membarrier, cmd, flags);
153:38.03                     ^~~~~~~~~~~~~~~
153:38.03                     membarrier
The __NR_membarrier define comes from unistd.h with a value that's processor-specific. There's no mention of it in the header in the SDK, but a quick search on the Web makes the reason clear: the value was introduced in kernel version 4.3. Sailfish OS runs various different kernel versions; the one on my Xperia 10 III is 4.19:
[defaultuser@kolbe ~]$ uname -a
Linux kolbe 4.19.248 #1 SMP PREEMPT Fri Jul 7 13:17:23 UTC 2023 aarch64 GNU/Linux
However, the latest headers available in the SDK are for Linux kernel 3.18. Curious to know why, I asked mal on IRC:
<flypig> mal: what's the reason Sailfish uses kernel headers 3.18 as opposed to something newer?
<mal> flypig: there was some talk about headers, we need to test that having newer headers won't enable some new things in some packages which would break devices with older kernels
<mal> I probably should do a test build of devel with new headers to see what happens
<flypig> mal: if you were to bump up the headers, what's the latest version you're likely to be able to increase them to?
<mal> preferrably at least 4.4 but I would like to go to even 5.4 or newer if possible
<mal> some LTS version
<mal> it really depends on what works

So right now the headers aren't available, but they may be available in the future.

Looking through the header files for newer kernels I can see the value needed seems to be architecture-specific, but we're only interested in the aarch64 variant (because all of this code is gated with an __aarch64 define, so I'm wondering whether I can just get away with defining it.

This might be a really bad idea. On the other hand, looking through the code, there doesn't seem any way around it short of reverting the entire upstream commit. Adding the value in will allow the code to compile, but could cause havoc at run time in case I choose the wrong value and end up calling a random syscall.

However, mal also helpfully pointed out that there are kernel headers for Linux 5.4 available; apparently these are needed for some native Sailfish OS builds. Would these be safe to use?
<mal> flypig: looking at the diff in that it checks for the kernel version at runtime
<flypig> mal: so you think would therefore be safe?
<mal> yes, I think it should be ok
<mal> for that case0

For now I'm just going to define the value, since (assuming I've got the right one) this should also be safe for the same reasons. I'll shift to moving the newer headers in due course.

I shouldn't forget to do this, because I can make the situation clear in the commit/patch that I create. So I'm going to add the value in and maybe, just maybe, the newer linux headers will be introduced in a future SDK release. Thanks go to mal for helping with this. Thankfully the changes get the build to move on again.

Next we have another NS_LITERAL_STRING error similar to that we saw on Day 10.
190:14.47 mobile/sailfishos/components
190:18.48 ${PROJECT}/gecko-dev/mobile/sailfishos/components/nsClipboard.cpp: 
          In member function ‘virtual nsresult nsEmbedClipboard::SetData(
          nsITransferable*, nsIClipboardOwner*, int32_t)’:
190:18.48 ${PROJECT}/gecko-dev/mobile/sailfishos/components/nsClipboard.cpp:64:30:
          error: ‘NS_LITERAL_STRING’ was not declared in this scope
190:18.48    root->SetPropertyAsAString(NS_LITERAL_STRING("data"), buffer);
190:18.48                               ^~~~~~~~~~~~~~~~~
190:18.49 ${PROJECT}/gecko-dev/mobile/sailfishos/components/nsClipboard.cpp:64:30:
          note: suggested alternative: ‘NS_EXTERNAL_VIS’
190:18.49    root->SetPropertyAsAString(NS_LITERAL_STRING("data"), buffer);
190:18.49                               ^~~~~~~~~~~~~~~~~
190:18.49                               NS_EXTERNAL_VIS
There is a difference this time though: this is NS_LITERAL_STRING whereas then it was NS_LITERAL_CSTRING (note the extra "C"). It's because of that extra letter that my checks for other instances didn't throw anything up previously. But the fix is the same: we can simply change all these to either wrap them with u..._ns in the case of string literals, or using the nsLiteralString constructor otherwise (note again the slight change: last time it was nsLiteralCString with an extra "C").

Having worked through all these cases the build gets another step further. Next we have the following:
184:35.72 mobile/sailfishos/components
184:39.33 ${PROJECT}/gecko-dev/mobile/sailfishos/components/nsClipboard.cpp:
          In member function ‘virtual nsresult nsEmbedClipboard::GetData(
          nsITransferable*, int32_t)’:
184:39.34 ${PROJECT}/gecko-dev/mobile/sailfishos/components/nsClipboard.cpp:90:16:
          error: invalid use of incomplete type ‘class nsIThread’
184:39.34      rv = thread->ProcessNextEvent(true, &processedEvent);
184:39.34                 ^~
184:39.34 In file included from ${PROJECT}/obj-build-mer-qt-xr/dist/include
184:39.34                  from ${PROJECT}/gecko-dev/mobile/sailfishos/components
184:39.34 ${PROJECT}/obj-build-mer-qt-xr/dist/include/MainThreadUtils.h:12:7:
          note: forward declaration of ‘class nsIThread’
184:39.34  class nsIThread;
184:39.34        ^~~~~~~~~
184:39.43 In file included from ${PROJECT}/gecko-dev/mobile/sailfishos
184:39.43                  from ${PROJECT}/gecko-dev/mobile/sailfishos
          /components/nsClipboard.cpp: :
184:39.43 ${PROJECT}/obj-build-mer-qt-xr/dist/include/nsCOMPtr.h:
          In instantiation of ‘void nsCOMPtr::assert_validity() [with T = nsIThread]’:
184:39.43 ${PROJECT}/obj-build-mer-qt-xr/dist/include/nsCOMPtr.h:477:5:
          required from ‘nsCOMPtr::nsCOMPtr() [with T = nsIThread]’
184:39.43 ${PROJECT}/gecko-dev/mobile/sailfishos/components/nsClipboard.cpp:86:23:
          required from here
184:39.43 ${PROJECT}/obj-build-mer-qt-xr/dist/include/nsCOMPtr.h:436:21: error:
          static assertion failed: nsCOMPtr only works for types with IIDs.
          Either use RefPtr; add an IID to your type with 
          the nsCOMPtr point to a base class with an IID.
184:39.43      static_assert(1 < sizeof(TestForIID(nullptr)),
184:39.43                    ~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Checking the nsIThread.h file the ProcessNextEvent() method is definitely there, so I'm guessing both these errors are consequences of the header file not being included. Now I've added it in, let's see if that helps.

We're left with a whole slew of errors. I've cut out most of them for brevity, but also because we're not going to be able to fix them all today: most we'll have to come back to tomorrow.

But the errors are actually really important: they're the most significant upstream change we've yet hit, and they deserve inclusion in full. But we'll come to that in due course. Here, at any rate, are the first few errors:
226:53.87 mobile/sailfishos
227:03.66 In file included from ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread
227:03.66 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread
          /EmbedLiteCompositorBridgeParent.h:29:7: error: cannot derive from
          ‘final’ base ‘mozilla::layers::CompositorBridgeParent’ in derived type 
227:03.66  class EmbedLiteCompositorBridgeParent : public mozilla::layers::CompositorBridgeParent
227:03.66        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
227:03.66 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread
          /EmbedLiteCompositorBridgeParent.h:58:16: error: ‘virtual void
           ::VsyncId)’ marked ‘override’, but does not override
227:03.66    virtual void CompositeToDefaultTarget(VsyncId aId) override;
227:03.66                 ^~~~~~~~~~~~~~~~~~~~~~~~
227:04.21 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread
          /EmbedLiteCompositorBridgeParent.cpp: In constructor ‘mozilla::embedlite
          (uint32_t, mozilla::layers::CompositorManagerParent*, mozilla
          ::CSSToLayoutDeviceScale, const TimeDuration&, const CompositorOptions&,
          bool, const IntSize&)’:
227:04.21 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread
          /EmbedLiteCompositorBridgeParent.cpp:60:16: error:
          ‘AddBoolVarCache’ is not a member of ‘mozilla::Preferences’
227:04.21    Preferences::AddBoolVarCache(&mUseExternalGLContext,
227:04.21                 ^~~~~~~~~~~~~~~
227:04.21 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread
          /EmbedLiteCompositorBridgeParent.cpp: In member function ‘void 
227:04.21 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread
          /EmbedLiteCompositorBridgeParent.cpp:111:5: error: ‘GLScreenBuffer’
          was not declared in this scope
227:04.21      GLScreenBuffer* screen = context->Screen();
227:04.21      ^~~~~~~~~~~~~~

That's actually only a quarter of the errors that were actually generated. There were a real lot of errors.

Starting from the top, the first problem is that the CompositorBridgeParent class is marked as final. Final classes are supposed to be classes that can't be inherited from; none of their constituent parts can be overridden. But EmbedLite wants EmbedLiteCompositorBridgeParent to subclass it.

This turns out not to be new, and there's already a patch 0018 with the title "Make it possible to extend CompositorBridgeParent" that removes the final annotation to allow this to happen.

The patch is really simple, just removing the final annotation from the class, and it applies without the need for any manual teasing. Nice! At least some of the other errors in the output above could well be caused by this one small change, although it's not clear that this will cover all of them. But the easiest way to clear this question up is to run the build again.
$ patch -d gecko-dev -p1 < rpm/0018-sailfishos-compositor-Make-it-possible-to-extend-Com.patch
patching file gfx/layers/ipc/CompositorBridgeParent.h
Hunk #1 succeeded at 308 (offset -4 lines).
It'll take another few hours for the build to complete and it's quite later here, so I'll have to wait to find out the result until the morning. For all the other posts, check out my full Gecko Dev Diary.


Uncover Disqus comments