#1699 Update FreeBSD support

Chiuso
aperto 5 mesi fa da OlCe1 · 12 commenti
OlCe1 5 mesi fa ha commentato

Hi,

Just in case you didn’t see the forum message (https://forum.palemoon.org/viewtopic.php?f=5&t=25625&p=204527#p205217).

Please have a look at https://repo.palemoon.org/OlCe1/UXP/commits/branch/FreeBSD-support so we can start on importing the code.

Happy to answer comments/questions.

Regards.

Hi, Just in case you didn't see the forum message (https://forum.palemoon.org/viewtopic.php?f=5&t=25625&p=204527#p205217). Please have a look at https://repo.palemoon.org/OlCe1/UXP/commits/branch/FreeBSD-support so we can start on importing the code. Happy to answer comments/questions. Regards.
mattatobin 5 mesi fa ha commentato
Collaboratori

There is at least one format error in the last big commit to mozjemalloc and i am extremely sceptical about such a large change and introduction of brand new code to it.

However, I am not qualified to evaluate such a huge change.

The rest looks sane enough.

@Moonchild and @athenian200 what say you?

Once this checks out and is approved I will manually land this to trunk following our commit form which will be the example to follow for you @OlCe1 in the future. So is that ALL that is required?


Compare reference: https://repo.palemoon.org/MoonchildProductions/UXP/compare/master...OlCe1:FreeBSD-support

There is at least one format error in the last big commit to mozjemalloc and i am extremely sceptical about such a large change and introduction of brand new code to it. However, I am not qualified to evaluate such a huge change. The rest looks sane enough. @Moonchild and @athenian200 what say you? Once this checks out and is approved I will manually land this to trunk following our commit form which will be the example to follow for you @OlCe1 in the future. So is that ALL that is required? ---- Compare reference: https://repo.palemoon.org/MoonchildProductions/UXP/compare/master...OlCe1:FreeBSD-support
mattatobin added the
OS: Other
label 5 mesi fa
mattatobin added the
C: Memory
label 5 mesi fa
mattatobin added the
C: Build System
label 5 mesi fa
mattatobin added the
High Risk
label 5 mesi fa
mattatobin Titolo modificato da FreeBSD support a Update FreeBSD support 5 mesi fa
OlCe1 5 mesi fa ha commentato
Autore

Which format error are you talking about? You mean “%zu” instead of “%u” when printing size_t stats?

The change is not “huge”, actually the gist of it is quite simple (the devil might be in the details, though).

More importantly, it does concern FreeBSD only. All other platforms are unaffected (even the introduction of ‘volatile’ for ‘malloc_initialized’ is a no-op for all platforms).

The only part where specific code is not as separated as I would have wanted is the addition of a condition in malloc_init_hard’s ifdefs and the introduction of “goto error” after ‘malloc_initializing’ has been set. It may be cleaner to use inline functions with conditional content to operate on the init lock. At least, I didn’t introduce new ifdefs (except the last one in this function), I just leveraged Windows’ ones.

Also, if you think there are too many ifdefs in mutex wrapping code, I can separate Linux and FreeBSD code instead.

Let me know if some things need refactoring.

Which format error are you talking about? You mean "%zu" instead of "%u" when printing size_t stats? The change is not "huge", actually the gist of it is quite simple (the devil might be in the details, though). More importantly, it does concern FreeBSD only. All other platforms are unaffected (even the introduction of 'volatile' for 'malloc_initialized' is a no-op for all platforms). The only part where specific code is not as separated as I would have wanted is the addition of a condition in malloc_init_hard's ifdefs and the introduction of "goto error" after 'malloc_initializing' has been set. It may be cleaner to use inline functions with conditional content to operate on the init lock. At least, I didn't introduce new ifdefs (except the last one in this function), I just leveraged Windows' ones. Also, if you think there are too many ifdefs in mutex wrapping code, I can separate Linux and FreeBSD code instead. Let me know if some things need refactoring.
OlCe1 5 mesi fa ha commentato
Autore

Force-pushed the change ‘%u’ => ‘%zu’.

Force-pushed the change '%u' => '%zu'.
mattatobin 5 mesi fa ha commentato
Collaboratori

It was just one of the preprocessor directives. An errant space

It was just one of the preprocessor directives. An errant space
OlCe1 5 mesi fa ha commentato
Autore

I looked again but don’t see any. May be in the penultimate commit, but I don’t think there is any in the last one.

I looked again but don't see any. May be in the penultimate commit, but I don't think there is any in the last one.
OlCe1 5 mesi fa ha commentato
Autore

Pushed a new commit on top of the existing ones with simplifications.

  1. Removed the ‘malloc_initializing’ flag I introduced earlier, which required a small change in ‘malloc_mutex_init’ (see commit message).
  2. Do not hook on Linux code for mutexes. Keep FreeBSD case separate.

This reduces the diff and makes the code easier to read, which should help the review.

Pushed a new commit on top of the existing ones with simplifications. 1. Removed the 'malloc_initializing' flag I introduced earlier, which required a small change in 'malloc_mutex_init' (see commit message). 2. Do not hook on Linux code for mutexes. Keep FreeBSD case separate. This reduces the diff and makes the code easier to read, which should help the review.
OlCe1 5 mesi fa ha commentato
Autore

So is that ALL that is required?

Yes, this is all there is to it.

> So is that ALL that is required? Yes, this is all there is to it.
Moonchild 5 mesi fa ha commentato
Proprietario

I’m not going to have the opportunity around the holidays to review this properly. Maybe I can have a look between Chrismas and New Year, but no promises.

I'm not going to have the opportunity around the holidays to review this properly. Maybe I can have a look between Chrismas and New Year, but no promises.
OlCe1 5 mesi fa ha commentato
Autore

No problem. The difficult details are normally all explained in the big comment section, but don’t hesitate to ask if something is unclear.

No problem. The difficult details are normally all explained in the big comment section, but don't hesitate to ask if something is unclear.
Moonchild 5 mesi fa ha commentato
Proprietario

Ok I have had some time to look at this and overall looks OK but I need to make it a proper review so please open a pull request that I can review with code comments.

Ok I have had some time to look at this and overall looks OK but I need to make it a proper review so please open a pull request that I can review with code comments.
mattatobin 5 mesi fa ha commentato
Collaboratori

So I guess that will be me so that the patches are conformed properly. Ok.

@OlCe1 you don’t have to do anything except watch and learn.

So I guess that will be me so that the patches are conformed properly. Ok. @OlCe1 you don't have to do anything except watch and learn.
mattatobin added a new dependency 5 mesi fa
Moonchild removed a dependency 5 mesi fa
Moonchild added a new dependency 5 mesi fa
OlCe1 5 mesi fa ha commentato
Autore

New PR at #1706.

New PR at https://repo.palemoon.org/MoonchildProductions/UXP/pulls/1706.
Moonchild closed this issue 5 mesi fa
Moonchild aggiunta alle pietre miliari (rimosso) 5 mesi fa
Moonchild pietra miliare modificata da (rimosso) a 29.0.0 4 mesi fa
Effettua l'accesso per partecipare alla conversazione.
Nessuna milestone
Nessuna assegnatario
3 Partecipanti
Notifiche
Data di scadenza

Nessuna data di scadenza impostata.

Dipende da
#1703 Add Modern FreeBSD Support
MoonchildProductions/UXP
Caricamento…
Non ci sono ancora contenuti.