Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid -DHUNSPELL_STATIC when using system hunspell #211

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

catap
Copy link

@catap catap commented Aug 6, 2024

No description provided.

@catap
Copy link
Author

catap commented Aug 7, 2024

It fixes #208

@rmottola
Copy link
Owner

rmottola commented Aug 7, 2024

However Firefox kept that in esr52 and esr60 - I wonder if this hack is needed on OpenBSD for those if somebody still compiles them?
It was removed - without explicitely saying to remove static - in : Bug 1463637

@catap
Copy link
Author

catap commented Aug 7, 2024

With this fix I was able to build artic-fox on OpenBSD -current/amd64 with config:

export CC="clang"
export CXX="clang++"
export LDFLAGS="-Wl,-zwxneeded"

mk_add_options MOZ_MAKE_FLAGS="-s -j2"

ac_add_options --disable-crashreporter
ac_add_options --disable-tests
ac_add_options --disable-debug
ac_add_options --disable-updater
ac_add_options --enable-mozril-geoloc
ac_add_options --disable-webrtc
ac_add_options --disable-safe-browsing
ac_add_options --disable-parental-controls
ac_add_options --enable-release
ac_add_options --disable-necko-wifi
ac_add_options --disable-eme
ac_add_options --disable-gamepad
ac_add_options --enable-dbus
ac_add_options --disable-gio
ac_add_options --disable-pulseaudio
ac_add_options --enable-strip
ac_add_options --enable-install-strip
ac_add_options --enable-application=browser
ac_add_options --with-branding=browser/branding/arcticfox
ac_add_options --enable-optimize="-O0 -g3"
ac_add_options --enable-default-toolkit=cairo-gtk2
ac_add_options --disable-precompiled-startupcache
ac_add_options --enable-system-hunspell

Maybe in openbsd config two things should be changed?

  1. Uncomment --enable-system-hunspell
  2. Add -O0 -g3 as optimization flags because it crashes without it, indeed.

@catap
Copy link
Author

catap commented Aug 7, 2024

However Firefox kept that in esr52 and esr60 - I wonder if this hack is needed on OpenBSD for those if somebody still compiles them? It was removed - without explicitely saying to remove static - in : Bug 1463637

I think that the same hack possible required for someone who compiles old Firefox on new OpenBSD.

I also think that the root cause is new LLVM, and not OpenBSD. But, who knows and I a bit lazy to dig into rootcause.

@rmottola
Copy link
Owner

rmottola commented Aug 7, 2024

-O0 is very very slow... last time I tried, -O1 was enough, with -O2 sometimes working sometimes not.
On GCC there are some specific optimisations which needs to be disabled, but also that only on certain systems, e.g. on linux gcc14 just works.
you can try standard setup with "ac_add_options --enable-optimize" without further specifying

@catap
Copy link
Author

catap commented Aug 7, 2024

you can try standard setup with "ac_add_options --enable-optimize" without further specifying

It crashes on start.

@catap
Copy link
Author

catap commented Aug 7, 2024

Hm, I just rebuild it with ac_add_options --enable-optimize="-g3" and it works

@catap
Copy link
Author

catap commented Aug 7, 2024

However Firefox kept that in esr52 and esr60 - I wonder if this hack is needed on OpenBSD for those if somebody still compiles them? It was removed - without explicitely saying to remove static - in : Bug 1463637

I just backported that patch into master as catap@8857ed1, and it fails the same way:

 0:36.18 /usr/include/c++/v1/locale:2827:22: error: no member named 'HunspellAllocator' in namespace 'std'; did you mean simply 'HunspellAllocator'?
 0:36.18     _Tp* __t = (_Tp*)std::realloc(__owns ? __b.get() : 0, __new_cap);
 0:36.18                      ^
 0:36.18 /home/catap/src/Arctic-Fox/extensions/spellcheck/hunspell/glue/mozHunspellAllocator.h:12:7: note: 'HunspellAllocator' declared here
 0:36.18 class HunspellAllocator : public mozilla::CountingAllocatorBase<HunspellAllocator>
 0:36.19       ^
 0:36.19 libembedding_components_commandhandler.a.desc
 0:36.19 1 error generated.
 0:36.19 

@rmottola
Copy link
Owner

rmottola commented Aug 8, 2024

Hm, I just rebuild it with ac_add_options --enable-optimize="-g3" and it works

that way you are optimizing nothing... I did test quite a bit on 7.4 and there -O1 was feasible

@rmottola
Copy link
Owner

rmottola commented Aug 8, 2024

However Firefox kept that in esr52 and esr60 - I wonder if this hack is needed on OpenBSD for those if somebody still compiles them? It was removed - without explicitely saying to remove static - in : Bug 1463637

I just backported that patch into master as catap@8857ed1, and it fails the same way:

 0:36.18 /usr/include/c++/v1/locale:2827:22: error: no member named 'HunspellAllocator' in namespace 'std'; did you mean simply 'HunspellAllocator'?
 0:36.18     _Tp* __t = (_Tp*)std::realloc(__owns ? __b.get() : 0, __new_cap);
 0:36.18                      ^
 0:36.18 /home/catap/src/Arctic-Fox/extensions/spellcheck/hunspell/glue/mozHunspellAllocator.h:12:7: note: 'HunspellAllocator' declared here
 0:36.18 class HunspellAllocator : public mozilla::CountingAllocatorBase<HunspellAllocator>
 0:36.19       ^
 0:36.19 libembedding_components_commandhandler.a.desc
 0:36.19 1 error generated.
 0:36.19 

oh, so it apparently doesn't fix....
I have an issue on FreeBSD with realloc - #212

@catap
Copy link
Author

catap commented Aug 9, 2024

Yeah, and as I point at #208 (comment) the LLVM triggers an issue.

The most interesting questions is why such issue hadn't happened in firefox at the first place.

@rmottola
Copy link
Owner

Yeah, and as I point at #208 (comment) the LLVM triggers an issue.

The most interesting questions is why such issue hadn't happened in firefox at the first place.

Indeed, that is a good point, why doesn't it happen in firefox? it is still static in latest version, so how is eg. building of seamonkey and firefox esr115 version working!

I backported a patch that should ensure system library gets used, but it doesn't. I I suppose "glue" is still needed^ how do you understand it?

@catap
Copy link
Author

catap commented Sep 20, 2024

@rmottola well... with git bisect and some time it is possible to discover when the build was fixed. If I recall right, they simple removes the code with this defines :) No defines, no problems.

@rmottola
Copy link
Owner

rmottola commented Sep 23, 2024

@catap if you like to try? I don't build current Firefox myself.

I think the issue might be in here:
https://github.com/rmottola/Arctic-Fox/blob/master/extensions/spellcheck/hunspell/glue/hunspell_alloc_hooks.h

So I found potentially this commit could be the "fix":
mozilla/gecko-dev@aadb161

but I can't backport it because we have no mozmemory.h available when building hunspell... may need some other bits.

https://github.com/rmottola/Arctic-Fox/blob/master/memory/build/moz.build

shows it is exported though...

@catap
Copy link
Author

catap commented Sep 23, 2024

Unfortently I also doesn't build firefox, and I have no idea how to build it.

Regarding your commit: I doubt that it will help because an issue is define itself, which replaces realloc inside std::realloc into std::HunspellAllocator::CountingRealloc inside some C++ headers.

The rigth fix is move away from enforcing this via -include .../mozilla-config.h and add it only after all system's headers is included.

Another way which may work is create a weapper functions like std::HunspellAllocator::CountingRealloc...

@rmottola
Copy link
Owner

@catap worth investigating though, because read around and fixes maybe undefs and swizzling header orders and we know current firefox has no issues, so a backport might be worth.
Interesting is that on linux with gcc - where I have no issues however - I am able to use the patch. Look

% find . -name mozmemory.h
./memory/build/mozmemory.h
./obj-x86_64-pc-linux-gnu/dist/include/mozmemory.h

On FreeBSD the file is not included in obj-dir, clear it does not get found:

% find . -name mozmemory.h
./memory/build/mozmemory.h

You might check on OpenBSD? Maybe the build order is different or it doens't get properly exported.. wonder why?

Further check shows MOZ_MEMORY being enabled on my Linux build:
obj-x86_64-pc-linux-gnu/dist/include/mozilla-config.h:#define MOZ_MEMORY 1

@catap
Copy link
Author

catap commented Sep 23, 2024

@rmottola I do have only one mozmemory.h, like you on FreeBSD.

I've tried this chnages:

--- a/extensions/spellcheck/hunspell/glue/hunspell_alloc_hooks.h
+++ b/extensions/spellcheck/hunspell/glue/hunspell_alloc_hooks.h
@@ -47,9 +47,10 @@
  * allocated using C memory allocation functions.
  */
 
-#include "mozilla/mozalloc.h"
 #include "mozHunspellAllocator.h"
 
+#include "mozmemory.h"
+
 #define malloc(size) HunspellAllocator::CountingMalloc(size)
 #define calloc(count, size) HunspellAllocator::CountingCalloc(count, size)
 #define free(ptr) HunspellAllocator::CountingFree(ptr)

which is similar with mozilla/gecko-dev@aadb161

And it fails as:

 1:00.10 In file included from /home/catap/src/Arctic-Fox/obj-amd64-unknown-openbsd7.6/mozilla-config.h:194:
 1:00.10 /home/catap/src/Arctic-Fox/extensions/spellcheck/hunspell/glue/hunspell_alloc_hooks.h:52:10: fatal error: 'mozmemory.h' file not found
 1:00.10 #include "mozmemory.h"
 1:00.10          ^~~~~~~~~~~~~

@rmottola
Copy link
Owner

@catap so OpenBSD like FreeBSD here. Did you check the file, like I did in #211 (comment) ? something is different in the build system.

I insist here, because reading bugzilla, this could be the fix, it fixes a very similar error on memory redefines!

@catap
Copy link
Author

catap commented Sep 24, 2024

Arctic-Fox $ git status
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
Arctic-Fox $ env AUTOCONF_VERSION=2.13 ./mach build                              
...
 0:42.29 gmake[4]: *** Waiting for unfinished jobs....
 0:42.29 2 warnings generated.
 0:42.29 liblayout_xul.a.desc
 0:42.29 gmake[3]: *** [/home/catap/src/Arctic-Fox/config/recurse.mk:33: compile] Error 2
 0:42.29 gmake[2]: *** [/home/catap/src/Arctic-Fox/config/rules.mk:540: default] Error 2
 0:42.29 gmake[1]: *** [/home/catap/src/Arctic-Fox/client.mk:411: realbuild] Error 2
 0:42.29 gmake: *** [client.mk:168: build] Error 2
 0:42.31 547 compiler warnings present.
Arctic-Fox $ Arctic-Fox $ find . -name mozmemory.h
./memory/build/mozmemory.h
Arctic-Fox $ find . -name mozilla-config.h
./obj-amd64-unknown-openbsd7.6/dist/include/mozilla-config.h
./obj-amd64-unknown-openbsd7.6/mozilla-config.h
Arctic-Fox $ grep MOZ_MEMORY ./obj-amd64-unknown-openbsd7.6/dist/include/mozilla-config.h
Arctic-Fox $ grep MOZ_MEMORY ./obj-amd64-unknown-openbsd7.6/mozilla-config.h  
Arctic-Fox $ 

So, nothing.

@rmottola
Copy link
Owner

@catap I think what you see is the issue why this fix doesn't apply! But.... why I don't know.

@rmottola
Copy link
Owner

it pisses me off that two BSDs are broken... but the issue here is Clang and is libc++... Disliked it. On FreeBSD gcc13 fails because it misses arc4random (old issue, that's why I used system clang). gcc14 instead fails configure tests... which I wonder, it works on Linux!!
You could try your luck on OpenBSD.

@catap
Copy link
Author

catap commented Sep 24, 2024

@rmottola yeah, building firefox is mess. It was mess, and it is mess now :(

@rmottola
Copy link
Owner

@catap I tried again on 7.5 i386. Clang fails with the known error, gcc with

127:40.50     INPUT("StaticXULComponentsEnd/StaticXULComponentsEnd.o")
127:40.50 
127:40.50 ld: error: undefined hidden symbol: arc4random
127:40.50 >>> referenced by jsmath.cpp

which is exactly as FreeBSD. So perhaps if we had one fix, it would be catch all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants