1785285 - firefox 104: alignment compiler error (freebsd 12.3/i386 - 32-bit)

Status: RESOLVED FIXED

(Toolkit :: General, defect, P5)

Description

Steps to reproduce:

Build firefox-104.0 on FreeBSD 12.x 32-bit (i386):

make -C /usr/ports/www/firefox build

Actual results:

Compiler error (alignment problem):

In file included from /usr/ports/www/firefox/work/firefox-1
04.0/devtools/shared/heapsnapshot/CoreDump.pb.cc:4:
In file included from /usr/ports/www/firefox/work/firefox-1
04.0/devtools/shared/heapsnapshot/CoreDump.pb.h:24:
In file included from /usr/ports/www/firefox/work/.build/dist/include/google/protobuf/arena.h:52:
/usr/ports/www/firefox/work/.build/dist/include/google/protobuf/arena_impl.h:643:10: error: requested alignment is less than minimum alignment of 8 for
 type 'google::protobuf::internal::ThreadSafeArena::CacheAlignedLifecycleIdGenerator'
  struct alignas(kCacheAlignment) CacheAlignedLifecycleIdGenerator {
         ^
1 error generated.

Comment 1

[John Hein]

See also https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=265905

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Firefox Build System::General' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Could you please share the full command line ?
./mach build -v
should give it

Comment 4

[John Hein]

The freebsd ports build phase uses 'gmake -f Makefile all'. Before that it runs 'configure'.

I tried doing ./mach build -v, and it started creating a new build environment. It's still chugging along doing cargo things. I don't think you want the output of all that.

Comment 5

[John Hein]

Attachment: .mozconfig

Maybe .mozconfig will help.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

i just wanted the full command line to build this file (CoreDump.pb.cc I guess)

Comment 7

[Mike Hommey [:glandium]]

kCacheAlignment is defined as such:

#ifdef __cpp_aligned_new
enum { kCacheAlignment = 64 };
#else
enum { kCacheAlignment = alignof(max_align_t) };  // do the best we can
#endif

I'd say your compiler environment is buggy if max_align_t has an alignment lower than that of other types.

Comment 8

[John Hein]

(In reply to Mike Hommey [:glandium] from comment #7)

kCacheAlignment is defined as such:

#ifdef __cpp_aligned_new
enum { kCacheAlignment = 64 };
#else
enum { kCacheAlignment = alignof(max_align_t) };  // do the best we can
#endif

I'd say your compiler environment is buggy if max_align_t has an alignment lower than that of other types.

alignof(max_align_t) = 4 on the FreeBSD-12/i386 architecture

Where does the minimum protobuf alignment of 8 come from?

Comment 9

[John Hein]

(In reply to Sylvestre Ledru [:Sylvestre] from comment #6)

i just wanted the full command line to build this file (CoreDump.pb.cc I guess)

Of course. Here you go.

/usr/local/bin/clang++13 -std=gnu++17 -o CoreDump.pb.o -c -I/usr/ports/www/firefox/work/.build/dist/stl_wrappers -I/usr/ports/www/firefox/work/.build/dist/system_wrappers -include /usr/ports/www/firefox/work/firefox-104.0/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fstack-clash-protection -DNDEBUG=1 -DTRIMMED=1 -DOS_POSIX=1 -DOS_FREEBSD=1 -DOS_BSD=1 -DGOOGLE_PROTOBUF_NO_RTTI -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/usr/ports/www/firefox/work/firefox-104.0/devtools/shared/heapsnapshot -I/usr/ports/www/firefox/work/.build/devtools/shared/heapsnapshot -I/usr/ports/www/firefox/work/.build/ipc/ipdl/_ipdlheaders -I/usr/ports/www/firefox/work/firefox-104.0/ipc/chromium/src -I/usr/ports/www/firefox/work/.build/dist/include -I/usr/local/include/nspr -I/usr/local/include -I/usr/local/include/nss -I/usr/local/include/nspr -I/usr/ports/www/firefox/work/.build/dist/include/nss -I/usr/local/include -I/usr/local/include/libpng16 -I/usr/local/include/pixman-1 -I/usr/local/include -DMOZILLA_CLIENT -include /usr/ports/www/firefox/work/.build/mozilla-config.h -Qunused-arguments -DLIBICONV_PLUG -isystem /usr/local/include -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wdeprecated-this-capture -Wempty-body -Wformat-type-confusion -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wno-error=tautological-type-limit-compare -Wunreachable-code -Wunreachable-code-return -Wunused-but-set-parameter -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wc++2a-compat -Wcomma -Wenum-compare-conditional -Wimplicit-fallthrough -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=free-nonheap-object -Wno-error=return-std-move -Wno-error=atomic-alignment -Wno-error=deprecated-copy -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-psabi -Wthread-safety -Wno-unknown-warning-option -fno-sized-deallocation -fno-aligned-new -O2 -pipe -O3 -DLIBICONV_PLUG -fstack-protector-strong -isystem /usr/local/include -fno-strict-aliasing -DLIBICONV_PLUG -isystem /usr/local/include -fno-exceptions -fPIC -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pipe -O2 -O3 -fomit-frame-pointer -funwind-tables -fno-strict-aliasing -ffp-contract=off -MD -MP -MF .deps/CoreDump.pb.o.pp /usr/ports/www/firefox/work/firefox-104. 0/devtools/shared/heapsnapshot/CoreDump.pb.cc

Comment 10

[Mike Hommey [:glandium]]

(In reply to John Hein from comment #8)

Where does the minimum protobuf alignment of 8 come from?

using LifecycleIdAtomic = uint64_t;
struct alignas(kCacheAlignment) CacheAlignedLifecycleIdGenerator {
  std::atomic<LifecycleIdAtomic> id;
};

So essentially, this comes from the required alignment from either uint64_t or std::atomic<uint64_t>

Comment 11

[John Hein]

(In reply to Mike Hommey [:glandium] from comment #10)

So essentially, this comes from the required alignment from either uint64_t or std::atomic<uint64_t>

alignof(uint64_t) = 4
alignof(std::atomic<uint64_t>) = 8

It's not clear to me that is a bug in the compiler environment or not.

from https://en.cppreference.com/w/cpp/types/max_align_t ...
"std::max_align_t is a trivial standard-layout type whose alignment requirement is at least as strict (as large) as that of every scalar type."

Are std::atomic products in the set of "every scalar type"?

Comment 12

[John Hein]

(In reply to John Hein from comment #11)

Are std::atomic products in the set of "every scalar type"?

std::is_scalar() says that std::atomic<uint64_t> is not a scalar. Nor is std::atomic<uint8_t>. Nor std::atomic<uint32_t>.

That seems to argue for the compiler environment not being buggy, but rather the expectation of the protobuf source code here that the architecture should be providing alignment at least as strictly as 8 bytes (instead of 4 bytes).

Comment 13

[Alex Hochheiden [:ahochheiden] [he/him]]

I followed up on this with Glandium, he confirmed it's a protobuf issue and not a build system issue. I'll re-assign this to toolkit since I believe that's what protobuff falls under.

Comment 14

[Marco Bonardo [:mak]]

Mark, is this something that should be reported upstream?

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Hey Mak, I suspect you got the wrong Mark?

Comment 16

[Marco Bonardo [:mak]]

I'm not sure, I looked at bug 1773604 that was the last protobuf update and it has r=markh, thus I wondered if you knew anything about how this library is updated, or someone who may know.

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Sorry, I missed that context. Since that bug, we've bumped to protobuf 21.6 in bug 1790929. https://github.com/protocolbuffers/protobuf/pull/10298 apparently changes the required alignment of strings and touched one of the bits of code implicated here - the issue mentions Android but I've no idea if it actually fixes this. That change does appear to be in 21.6 - so I guess the first question is whether this still exists on current central and whether the symptoms are the same?

If not, then I guess that repo would be the correct place to report the bug. I suspect it would be possible to revert back to an older protobuf if necessary - Ryan, any thoughts?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Yeah, that'd be the right place to report the issue if it's still happening. Can't say I'd be super thrilled about reverting the update on our end for a Tier 3 platform, though.

The commit that Mark linked to shipped in protobuf 21.4, which we took for Fx105 in bug 1782859. Would indeed be good to know if this still reproduces in 105+.

Comment 19

[Florian Quèze [:florian]]

My understanding of comment 18 is that this has been fixed for Firefox 105. Feel free to reopen if this is incorrect.