Re: MOZ_ASSUME_UNREACHABLE is being misused
In order to provide more solid technical background for this conversation,
I've studied a bit the practical effect of __builtin_unreachable on some
test programs, in GCC 4.6 and in Clang 3.4.
All my files are here,
https://github.com/bjacob/builtin-unreachable-study
The results are in the 'notes' file (and also pasted below for the
archives),
https://raw.githubusercontent.com/bjacob/builtin-unreachable-study/master/notes
The quick summary of conclusions is that:
- Clang 3.4 was not aggressive with taking advantage unreachable markers,
compared to GCC.
- GCC 4.6, on the other hand, was very aggressive and very smart taking
advantage of unreachable marker, producing code that was faster, smaller...
and very dangerous if the unreachable marker was hit. In particular, GCC
4.6 used unreachable markers to:
* Remove jump table bounds checks (See a.cpp; allowing to abuse a jump
table to jump to an arbitrary address);
* Jump to the end of a function without a 'ret' instruction (See b.cpp
silently continuing execution beyond the end of the function (potentially
doing exploitable things if not crashing immediately);
* Use unreachable markers in if() branches to infer that a condition is
never or always met, and use that to cleverly optimize away *other* if()
branches (See c.cpp and d.cpp).
The below is a copy of the 'notes' files containing the details:
*** notes file ***
This is a study of the effect of __builtin_unreachable (abbreviated as
just 'unreachable' below) in GCC 4.6 and in Clang 3.4, on Linux
x86_64, on four test programs.
It was made to provide some background for this mozilla.dev.platform
thread:https://groups.google.com/forum/#!topic/mozilla.dev.platform/E0sg9EYk1xU
*
*
Test program 1/4 - a.cpp
Switch statement with unreachable default case.
Source code:
#ifdef USE_UNREACHABLE
#define UNREACHABLE() __builtin_unreachable()
#else
#define UNREACHABLE()
#endif
int foo(int x, int y)
{
int result = 0;
switch(x) {
case 0: break;
case 1: result = y; break;
case 2: result = y*y + y; break;
case 3: result = y*y*y; break;
case 4: result = y*y + 1; break;
case 5: result = y*y*y + y; break;
default: UNREACHABLE(); break;
}
return result;
}
Results:
Both compilers implement the switch as a jump table.
Clang3.4: unreachable has no effect. The switch parameter is always
checked before using the jump table.
GCC4.6: unreachable has the effect of removing the check that the
switch parameter is in range, before using the jump table. Passing a
bad parameter value then allows to jump to an arbitrary address.
Here's the relevant part of the assembly with GCC 4.6:
Without unreachable:
cmpl$5, %edi
jbe .L11; check the switch parameter
.L2:
xorl%eax, %eax
ret ; out-of-range parameter, return early
.p2align 4,,10
.p2align 3
.L11:
movl%edi, %edi
jmp *.L8(,%rdi,8) ; using the jump table here
We see that if the switch parameter is out of range, we fall through
into .L2, returning from the function.
Now with unreachable:
cmpl$5, %edi
jbe .L12; check the switch parameter ...
.p2align 4,,10 ; ... but if it's not in range, we
don't early-return...
.p2align 3 ; ... so we keep executing through
some alignment padding ...
.L12: ; ... and we fall through ...
movl%edi, %edi
jmp *.L9(,%rdi,8) ; ... to the jump table!
We see that unreachable has the effect of removing the early-return in
the case where the switch parameter is bad. We thus carry on executing
from the current location, which takes us through some alignment
padding (.p2align). If the location is already aligned so that there
are no padding bytes here, or if the padding is filled with NOP's
(which according to
https://sourceware.org/binutils/docs/as/P2align.html is the case on
some systems), then we'll use the jump table with our bad parameter!
Conclusion: if we hit the 'unreachable' path, with clang3.4 we're
safe, but with gcc4.6 we have at least a crash, and a sec-critical if
an attacker can control the switch parameter.
*
*
Test program 2/4 - b.cpp
If/Else chain long suitable to be optimized as a jump table.
Source code:
#ifdef USE_UNREACHABLE
#define UNREACHABLE() __builtin_unreachable()
#else
#define UNREACHABLE()
#endif
int foo(int x, int y)
{
int result = 0;
if (x == 0) result = 1;
else if (x == 1) result = y;
else if (x == 2) result = y*y + y;
else if (x
Controlling font size as per system DPI in Windows
Hi, We have a property in greprefs.js layout.css.dpi by which we can control pixel size , but this doesn't work in Windows. There is another property layout.css.devPixelsPerPx which control font size in Windows but this needs to be modified each time as per system DPI. I need to show font size as per system DPI. How can this be achieved? I am using gecko SDK 14.0.1. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Controlling font size as per system DPI in Windows
On 1/4/14 13:05, bhargava.animes...@gmail.com wrote: Hi, We have a property in greprefs.js layout.css.dpi by which we can control pixel size , but this doesn't work in Windows. There is another property layout.css.devPixelsPerPx which control font size in Windows but this needs to be modified each time as per system DPI. I need to show font size as per system DPI. How can this be achieved? I am using gecko SDK 14.0.1. Set layout.css.devPixelsPerPx to -1.0. However, I expect this will be quite buggy in 14.0.1; you need a newer version of Gecko (at least Gecko 22 or thereabouts, IIRC) for better Windows DPI-setting support. See https://bugzilla.mozilla.org/show_bug.cgi?id=820679 and its dependencies. JK ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: MOZ_ASSUME_UNREACHABLE is being misused
2014-04-01 3:58 GMT-04:00 Benoit Jacob jacob.benoi...@gmail.com: * Remove jump table bounds checks (See a.cpp; allowing to abuse a jump table to jump to an arbitrary address); Just got an idea: we could market this as WebJmp, exposing the jmp instruction to the Web ? We could build a pretty strong case for it, we already ship it. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
mozilla::NullptrT on B2G ICS vs. everything else (incl. B2G JB and Android ICS)
While working on https://bugzilla.mozilla.org/show_bug.cgi?id=980317 , I discovered that support for mozilla::NullptrT and nullptr is worse on B2G ICS emulator than on other platforms, including B2G JB and Android ICS. See https://tbpl.mozilla.org/php/getParsedLog.php?id=35791355tree=Tryfull=1 for the errors. I would expect a) the compiler for B2G ICS to be at least as advanced as the compiler for Android ICS b) the compiler for B2G ICS not to be less advanced than the compiler for B2G JB This makes me wonder if B2G ICS emulator uses a nullptr-wise backwards compiler / compiler settings by accident. Can it be changed to use a compiler / compiler settings similar to B2G JB and Android ICS? This is https://bugzilla.mozilla.org/show_bug.cgi?id=980824 -- Henri Sivonen hsivo...@hsivonen.fi https://hsivonen.fi/ ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::NullptrT on B2G ICS vs. everything else (incl. B2G JB and Android ICS)
On 4/1/2014 8:42 AM, Henri Sivonen wrote: This makes me wonder if B2G ICS emulator uses a nullptr-wise backwards compiler / compiler settings by accident. Can it be changed to use a compiler / compiler settings similar to B2G JB and Android ICS? B2G ICS uses gcc 4.4, IIRC. All of the Android builds use 4.7, and it sounds like B2G JB uses 4.7 as well. gcc 4.4 is very lacking in several regards (no nullptr, and rvalue reference support is missing several issues), so I would very much love to see us using gcc 4.7 on b2g, but I've been told that partner builds won't like that. -- Joshua Cranmer Thunderbird and DXR developer Source code archæologist ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: MOZ_ASSUME_UNREACHABLE is being misused
2014-03-28 17:14 GMT-04:00 Benoit Jacob jacob.benoi...@gmail.com: 2014-03-28 16:48 GMT-04:00 L. David Baron dba...@dbaron.org: On Friday 2014-03-28 13:41 -0700, Jeff Gilbert wrote: My vote is for MOZ_ASSERT_UNREACHABLE and MOZ_OPTIMIZE_FOR_UNREACHABLE. It's really handy to have something like MOZ_ASSERT_UNREACHABLE, instead of having a bunch of MOZ_ASSERT(false, Unreachable.) lines. Consider MOZ_ASSERT_UNREACHABLE being the same as MOZ_OPTIMIZE_FOR_UNREACHABLE in non-DEBUG builds. I agree on the first (adding a MOZ_ASSERT_UNREACHABLE), but I don't think MOZ_OPTIMIZE_FOR_UNREACHABLE sounds dangerous enough -- the name should make it clear that it's dangerous for the code to be reachable (i.e., the compiler can produce undefined behavior). MOZ_DANGEROUSLY_ASSUME_UNREACHABLE is one idea I've thought of for that, though it's a bit of a mouthful. I too agree on MOZ_ASSERT_UNREACHABLE, and on the need to make the new name of MOZ_ASSUME_UNREACHABLE sound really scary. I don't mind if the new name of MOZ_ASSUME_UNREACHABLE is really long, as it should rarely be used. If SpiderMonkey gurus find that they need it often, they can always alias it in some local header. I think that _ASSUME_ is too hard to understand, probably because this doesn't explicitly say what would happen if the assumption were violated. One has to understand that this is introducing a *compiler* assumption to understand that violating it would be Undefined Behavior. How about MOZ_ALLOW_COMPILER_TO_GO_CRAZY ;-) This is technically correct, and explicit! Let's see if we can wrap up this conversation soon now. How about: MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE The idea of _COMPILER_ here is to clarify that this macro is tweaking the compiler's own view of the surrounding code; and the idea of _BELIEVE_ here is that the compiler is just going to believe us, even if we say something absurd, which I believe underlines our responsibility. I'm not a native English speaker so don't hesitate to point out any awkwardness in this construct... And as agreed above, we will also introduce a MOZ_ASSERT_UNREACHABLE macro doing MOZ_ASSERT(false, msg) and will recommend it for most users. If anyone has a better proposal or a tweak to this one, speak up! I'd like to be able to proceed with this soon. Benoit Benoit -David -- 턞 L. David Baron http://dbaron.org/ 턂 턢 Mozilla https://www.mozilla.org/ 턂 Before I built a wall I'd ask to know What I was walling in or walling out, And to whom I was like to give offense. - Robert Frost, Mending Wall (1914) ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: MOZ_ASSUME_UNREACHABLE is being misused
On 4/1/2014 10:54 AM, Benoit Jacob wrote: Let's see if we can wrap up this conversation soon now. How about: MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE I counter-propose that we remove the macro entirely. I don't believe that the potential performance benefits we've identified are worth the risk. --BDS ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: MOZ_ASSUME_UNREACHABLE is being misused
2014-04-01 10:57 GMT-04:00 Benjamin Smedberg benja...@smedbergs.us: On 4/1/2014 10:54 AM, Benoit Jacob wrote: Let's see if we can wrap up this conversation soon now. How about: MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE I counter-propose that we remove the macro entirely. I don't believe that the potential performance benefits we've identified are worth the risk. I certainly don't object to that, but I didn't suppose that I could easily get consensus around that. This macro is especially heavily used in SpiderMonkey. Maybe SpiderMonkey developers could weigh in on how large are the benefits brought by UNREACHABLE there? Benoit ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: MOZ_ASSUME_UNREACHABLE is being misused
On 4/1/14, 9:57 AM, Benjamin Smedberg wrote: On 4/1/2014 10:54 AM, Benoit Jacob wrote: Let's see if we can wrap up this conversation soon now. How about: MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE I counter-propose that we remove the macro entirely. I don't believe that the potential performance benefits we've identified are worth the risk. I agree. What should we replace MOZ_ASSUME_UNREACHABLE with? If the code is truly unreachable, it doesn't matter what we replace it with. The question is what to do when the impossible occurs. To me, letting control flow past such a place is just as scary as undefined behavior. So I think our replacement for MOZ_ASSUME_UNREACHABLE should crash even in non-debug builds. This suggests MOZ_CRASH. -j ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Proposal: External Protocol Handing (Plugin replacement)
Hi! As you want to get rid of all plugins (NPAPI), you say: If there are plugin features which are not available in the web platform, we encourage developers to post their use cases to mozilla.dev.platform project list”. https://developer.mozilla.org/en-US/Add-ons/Plugins We have such a feature: We currently use a plugin to detect whether one of our applications is installed and then launch it via iframe. This will no longer be possible without plugins, while providing a good user experience. My colleague Ben is currently proposing this API to public-weba...@w3.org, modeled after Win8/IE11’s navigator.msLaunchUri: http://bengjohnson.github.io/ExternalProtocolSpecification.html http://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0791.html msLaunchUri MSDN docs: http://msdn.microsoft.com/en-us/library/ie/jj154912(v=vs.85).aspx Would you consider such an API for Firefox? Cheers, Kosta PS: Sorry for the long signature... -- Konstantin Welke Senior Software Developer Citrix Online Germany GmbH | Erzbergerstr. 117 | D-76133 Karlsruhe T: +49 721 3544990 | F: +49 721 354499624 | M: +49 151 23429318 konstantin.we...@citrix.com http://www.citrixonline.com http://www.citrixonline.com/ http://www.citrixonline.com/ Work better. Live better. Citrix Online Germany GmbH | Erzbergerstr. 117 | D-76133 Karlsruhe Geschäftsführer: Tommy Ahlers | Michael DiFilippo | David Zalewski Sitz der Gesellschaft: Karlsruhe | Registergericht: Amtsgericht Mannheim HRB 713721 Citrix Online UK Ltd http://www.citrixonline.com/imprint-en.tmpl ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: MOZ_ASSUME_UNREACHABLE is being misused
On 4/1/14, 10:05 AM, Benoit Jacob wrote: This macro is especially heavily used in SpiderMonkey. Maybe SpiderMonkey developers could weigh in on how large are the benefits brought by UNREACHABLE there? I don't believe there are any benefits. Those uses of MOZ_ASSUME_UNREACHABLE are not intended as optimizations. When they were inserted, they used JS_NOT_REACHED, which called the assertion failure code and crashed hard, even in non-debug builds. Bug 723114 changed the semantics of the existing macro. Later, the name was changed. In hindsight, that was a big safety regression. By all means back it out. -j ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Enable -Wswitch-enum? [was Re: MOZ_ASSUME_UNREACHABLE is being misused]
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256
This is a bit of a tangent, but: There are a whole bunch of places in
layout, and probably elsewhere, where we have the following catch-22:
if you have a switch statement over the values of an enumeration, gcc
and clang (with -Wall) will warn you about enumeration values that are
not handled, but only if there is no default: clause. In basically
all of the instances I know about, default: MOZ_CRASH() would be
appropriate, but has been omitted in the name of continuing to get
that warning (many of these enumerations do regularly get new values
added).
I had been meaning for some time to file a feature request with the
gcc people for some way to get the warning even with the default case
present, but it turns out it's already there: -Wswitch-enum.
$ cat test.c
enum Foo { Bar, Baz, Quux };
extern void a(void);
extern void b(void);
extern void c(void);
void dispatch_1(enum Foo x)
{
switch (x) {
case Bar: a(); break;
case Baz: b(); break;
default: __builtin_trap();
}
}
void dispatch_2(enum Foo x)
{
switch (x) {
case Bar: a(); break;
case Baz: b(); break;
}
}
$ gcc -Wall -Wextra -fsyntax-only test.c
test.c: In function ‘dispatch_2’:
test.c:18:3: warning: enumeration value ‘Quux’ not handled in switch
[-Wswitch]
switch (x) {
^
$ gcc -Wall -Wextra -Wswitch-enum -fsyntax-only test.c
test.c: In function ‘dispatch_1’:
test.c:9:3: warning: enumeration value ‘Quux’ not handled in switch
[-Wswitch-enum]
switch (x) {
^
test.c: In function ‘dispatch_2’:
test.c:18:3: warning: enumeration value ‘Quux’ not handled in switch
[-Wswitch]
switch (x) {
^
Similarly for clang. Dunno about MSVC, but the cases of this that I
know about are all in OS-independent code so it wouldn't be a problem.
The downside of turning this on would be that any switch statements
that *deliberately* include only a subset of the enumerators, plus a
default case, would now have to be expanded to cover all the
enumerators. I would try it and report on how unpleasant that turns
out to be, but my development box has taken to corrupting its
filesystem if I merely do a 'hg update', so I'm kind of stuck, there.
Anyone interested in trying it?
zw
-BEGIN PGP SIGNATURE-
Version: GnuPG v1
Comment: Using GnuPG with Icedove - http://www.enigmail.net/
iQIcBAEBCAAGBQJTOuGbAAoJEJH8wytnaapke6YP/i1yyvKeWMbXX6b6eIa2Dv/v
njT6gRztp6V3LzreI8ShFNoUpt++Grvr1ZjO3cfpVOHujnq6GT4tJLG6KHI/E3MH
DdsAs6BdirmNwDhsWElQhZqtZ1f0ncu3/u2+tC2yOWEEkkTtJwjx7wBDQLZDrRdz
q5e0gE0wLh8pK1qf+LrU36o6N8T2+GLgqbEkNn/RnfGWINxG4PINm8EAZuFMnGt8
0Lpa9W03YVr1c53LpnH80jsfZ8JL4zszD7Sw1clTZgQEGeP31OXZaRZIFk+OAp3P
Yo13utlWb+AeSL8uQtQwW55YQAdJxx+fi7o+1szR/AgUfPs32dL6kmCIxcxx/eUb
0E1kmREhtmUKRA3DAM7SQ6DVXAS4TS+Rxa0HutLHHywU6GabNTeEMdyM8bseet+y
SlPqXf9o4UIwUuE9Sv+Jxvp9QnoIP6tFwyu99NVGDyPK6OoVRH4rjTeszxQckFs2
wjGmS0yGAsNgMcRHgav2CDvJVL+R+H0irgXQPaEl0CjonI9lGFYivp6tFZDgmDmI
u4sK9PEHhX/aECOqZgjexbS2QZGa7PcLjNfB251kl5/Oa+dTMNsvFH/pHm7s8rrZ
c/02VCzpl2b/qVkcwzggCZIFFu8J66YiniZlpnCBqrPpOcIGhpgMRJgbYSavgWSx
kwELrg4z3S+dsOs6x6zb
=b0ly
-END PGP SIGNATURE-
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::NullptrT on B2G ICS vs. everything else (incl. B2G JB and Android ICS)
On Tue, Apr 01, 2014 at 09:47:15AM -0500, Joshua Cranmer ? wrote: On 4/1/2014 8:42 AM, Henri Sivonen wrote: This makes me wonder if B2G ICS emulator uses a nullptr-wise backwards compiler / compiler settings by accident. Can it be changed to use a compiler / compiler settings similar to B2G JB and Android ICS? B2G ICS uses gcc 4.4, IIRC. All of the Android builds use 4.7, and it sounds like B2G JB uses 4.7 as well. gcc 4.4 is very lacking in several regards (no b2g jb using gcc 4.7 was what I guessed as well, but http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-hamachi/1396314004/b2g_mozilla-central_hamachi_dep-bm72-build1-build10.txt.gz seems to provide some evidence to the contrary. configure says it finds gcc 4.4, but b2g 'helpfully' runs make with -s so we can't know what actually happens... Trev nullptr, and rvalue reference support is missing several issues), so I would very much love to see us using gcc 4.7 on b2g, but I've been told that partner builds won't like that. -- Joshua Cranmer Thunderbird and DXR developer Source code archæologist ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform signature.asc Description: Digital signature ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Enable -Wswitch-enum? [was Re: MOZ_ASSUME_UNREACHABLE is being misused]
On 04/01/2014 08:56 AM, Zack Weinberg wrote: The downside of turning this on would be that any switch statements that *deliberately* include only a subset of the enumerators, plus a default case, would now have to be expanded to cover all the enumerators. I would try it and report on how unpleasant that turns out to be, but my development box has taken to corrupting its filesystem if I merely do a 'hg update', so I'm kind of stuck, there. Anyone interested in trying it? As a first approximation, it seems like we'd have to do this for a most switch statements that include a 'default:' case. (I'm betting that most of those switch statements have omitted *some* enum values, since until now, there's been no reason to include 'default' unless you're trying to be concise and merge some values together.) grep finds 3934 default statements in .cpp files, 469 in .h files: $ grep -r default: | grep .cpp: | wc -l 3934 $ grep -r default: | grep .h: | wc -l 469 So, we have on the order of ~4400 switch statements that would potentially need expanding to avoid tripping this warning. Having said that, if we *really* wanted to add this warning, I suppose we could add it directory-by-directory (or only ever add it in certain directories, as is the case with -Wshadow in layout/style). ~Daniel ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal: External Protocol Handing (Plugin replacement)
Hi!
Well, I think adding an API to query directly whether an application is
installed could be a privacy risk. This is why Microsoft added this API to
obscure this information to the web page and just call either the success
or failure callback.
In Citrix, we use this API on IE 11 / Win 8 to e.g. launch our programs
(if installed) or provide the user with a download (if the user declined
the launching / program is not installed). Currently, we use a plugin on
Firefox, Chrome, Safari for the same functionality. For environments where
users cannot typically install programs (Android, iOS, Windows RT, etc.),
we provide a link to the App Store / Play Store / Windows Store instead of
providing a direct download.
Made-up example for launching SSH:
(this works on IE 11 / Win 8 if you use navigator.msLaunchUri):
navigator.launchUri(“ssh://user@host”,
function successCallback() {
//show some UI to show success
},
function failureCallback() {
//display UI to explain how to install an SSH client or re-try
});
One of the main advantages over a plain a href=“ssh://user@host”... is
that if no “ssh://“ scheme handler is installed and the user clicks that
link, there is reliable specified behavior (the failureCallback). Using
currently existing means, that experience is pretty bad and very different
in each browser (e.g. Chrome just ignores the click (after firing unload
events), Firefox asks the user which app to launch (without showing a
default app), Safari/OSX asks to search the app store, etc.). The web page
does not know whether it worked and cannot help the user. For a good user
experience, the page should know whether launching the app worked or not
and be allowed to handle the UI - instead of the browser showing something
random.
Sidenote: Chrome on Android has this functionality using intents, using
the following style:
intent://urltolaunch#Intent;scheme=schemetolaunch;package=packagetoinstall;
end, which either launches schemetolaunch://urltolaunch to redirects to
the play store to install packagetoinstall.
Cheers,
Kosta
On 4/1/14, 7:01 PM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote:
Hi Konstantin,
So, is the main use case here determining whether the application is
installed? What URL schemes do you use for this? What kind of UX do
you present to the user if the application is not installed?
Also, how does this picture fit into environments where the application
is not even provided (hypothetically speaking, let's say on Android)?
Thanks!
Ehsan
On 2014-04-01, 11:30 AM, Konstantin Welke wrote:
Hi!
As you want to get rid of all plugins (NPAPI), you say: If there are
plugin features which are not available in the web platform, we
encourage
developers to post their use cases to mozilla.dev.platform project
list”.
https://developer.mozilla.org/en-US/Add-ons/Plugins
We have such a feature: We currently use a plugin to detect whether one
of
our applications is installed and then launch it via iframe. This will
no
longer be possible without plugins, while providing a good user
experience.
My colleague Ben is currently proposing this API to
public-weba...@w3.org,
modeled after Win8/IE11’s navigator.msLaunchUri:
http://bengjohnson.github.io/ExternalProtocolSpecification.html
http://lists.w3.org/Archives/Public/public-webapps/2014JanMar/0791.html
msLaunchUri MSDN docs:
http://msdn.microsoft.com/en-us/library/ie/jj154912(v=vs.85).aspx
Would you consider such an API for Firefox?
Cheers,
Kosta
PS: Sorry for the long signature...
--
Konstantin Welke
Senior Software Developer
Citrix Online Germany GmbH | Erzbergerstr. 117 | D-76133 Karlsruhe
T: +49 721 3544990 | F: +49 721 354499624 | M: +49 151 23429318
konstantin.we...@citrix.com
http://www.citrixonline.com http://www.citrixonline.com/
http://www.citrixonline.com/
Work better. Live better.
Citrix Online Germany GmbH | Erzbergerstr. 117 | D-76133 Karlsruhe
Geschäftsführer: Tommy Ahlers | Michael DiFilippo | David Zalewski
Sitz der Gesellschaft: Karlsruhe | Registergericht: Amtsgericht Mannheim
HRB 713721
Citrix Online UK Ltd http://www.citrixonline.com/imprint-en.tmpl
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: MOZ_ASSUME_UNREACHABLE is being misused
Jason Orendorff writes: If the code is truly unreachable, it doesn't matter what we replace it with. The question is what to do when the impossible occurs. To me, letting control flow past such a place is just as scary as undefined behavior. That depends on the particular code. Often enough, letting control flow past is relatively harmless compared to a crash. So I think our replacement for MOZ_ASSUME_UNREACHABLE should crash even in non-debug builds. This suggests MOZ_CRASH. For many of our assertions, letting processing continue when an assertion is not satisfied is just as scary as undefined behaviour. I'm not aware of any reason to treat MOZ_ASSERT_UNREACHABLE differently from MOZ_ASSERT in release builds. Perhaps all asserts should crash by default in release builds. But, as a crash may sometimes be worse than the consequences of a particular assertion failure, perhaps we need to distinguish, ok-to-continue asserts from do-not-continue asserts. Even if we had only the do-not-continue asserts, including array bounds checks, crash in release builds, I don't know the performance or code size costs. I'm guessing we'd still need some dont-want-to-continue-but-too-expensive-to-check variant of assert. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Enable -Wswitch-enum? [was Re: MOZ_ASSUME_UNREACHABLE is being misused]
Zack Weinberg writes: This is a bit of a tangent, but: There are a whole bunch of places in layout, and probably elsewhere, where we have the following catch-22: if you have a switch statement over the values of an enumeration, gcc and clang (with -Wall) will warn you about enumeration values that are not handled, but only if there is no default: clause. In basically all of the instances I know about, default: MOZ_CRASH() would be appropriate, but has been omitted in the name of continuing to get that warning (many of these enumerations do regularly get new values added). Does WARNINGS_AS_ERRORS make the default:MOZ_CRASH() unnecessary? My initial impression from cairo code, which uses -Wswitch-enum, is that it can be burdensome, so an optional solution, such as choosing whether to include the default clause, sounds appealing. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Spring cleaning: Reducing Number Footprint of HG Repos
Wow, okay. A lot to address here. The primary instigator of this migrating of user repositories off to external services came from when we were (and still are) crunched for disk space after restructuring our Mercurial infrastructure to use local disks. We did this for several reasons: * An internal quote for remaining on NFS for hosting (even with just the 300GB used) would have cost us a low six-digit figure * Mercurial devs originally said that some of our clone corruption problems may have come from NFS faking transaction atomicity (see bug 974094) * This approach did not allow us to expand to multiple datacenters (especially cost-effectively) The 300GB limit in this case comes from repurposing the old hgweb-serving hosts to use their local disks instead of an NFS mount. These hosts came with two 300GB disks paired in RAID-1 configuration. If this is simply a matter of disk space we can agree to reconfigure these hosts as RAID-0 instead. The reliability should never matter since these are simply clones of the original canonical source. This is what I was spending a considerable amount of time doing. Additionally I've been setting up a host named hg-archive.mozilla.org with a lower SLA to shelve repositories that have not been touched in many many years. Deleting this old code from hg.m.o, even if it's available elsewhere if an unpopular thing to do, so it's unsurprising I didn't receive much buy-in when I proposed it. I think the real reason for this evaluation and push into other services is that it is perceived that the user repositories don't add much value, especially when you consider all of the features that could be happening from them such as triggering CI jobs based on these, and self-service collaboration. Yes, the user-repo deletion is a feature and it is currently broken. It's been a corner-case of the migration to local disk, and a fix has yet to be coded up. Please ping me if you're trying to remove a repository until I can fix this. As for project repositories, these should totally be self-service and automated. The human-as-an-API approach to these means it is often too much work for developers to request one for simple or short collaborative projects. Sadly for Mercurial development at Mozilla it is just me for the development work. If, as gps said, people are willing to help out with some of the development I would be happy to test and deploy whatever changes are proposed. The code for the infrastructure is available at https://github.com/bkero/puppet-module-hg. Feel free to spin up a VM and try to improve things. Ben ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Fwd: [Planned Maintenance Notification] 4/5 Tree Closing Maintenance Window
FYI: (bcc: release@, sheriffs@) Original Message Subject:[Planned Maintenance Notification] 4/5 Tree Closing Maintenance Window Date: Tue, 01 Apr 2014 20:32:56 - Short Summary: Mozilla IT will have a tree closing maintenance window on Saturday, 4/5, from 0700 PDT - 1000 PDT. During this window a few minor network changes and database master changes will be made to improve our services. Following is the work to be completed: 985503 - [tracker bug] 982293 - bugzilla database master failover 985745 - clean up network switch/router configurations 987992 - tbpl database master failover 988202 - upgrade releng switches 988288 - NFS volume splits for product delivery 988384 - generic database master failover Mozilla IT Outage Notification: -- Issue Status: Upcoming Bug IDs: 985503 Start Date:2014-04-05 Start Time:07:00 PDT Site: SJC1 Services: Tree Closure Impact of Work:**During this period of time all development trees will be closed.** If you have any questions or concerns please address them to incide...@mozilla.com -- ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
mozilla::Atomic considered harmful
The subject of this post is intentionally chosen to make you want to read
this. :-)
The summary is that I think the mozilla::Atomic API which is modeled after
std::atomic is harmful in that it makes it very non-obvious that you're
dealing with atomic integers. Basically the existing interface makes
mozilla::Atomic look like a normal integer, so that if you see code like:
--mRefCnt;
if (mRefCnt == 0) {
delete this;
}
You won't immediately think about checking the type of mRefCnt (the
refcount case being just an example here of course), which makes it hard to
spot that there is a thread safety bug in this code. Given that I and
three experienced Gecko hackers fell prey to this issue (bug 985878 and bug
987667), I thought about the underlying problem a bit, and managed to
convince myself that it's a bad idea for us to pretend that atomic integers
can be treated the same way as non-atomic integers.
So, over in bug 987887 I'm proposing to remove all of the methods on
mozilla::Atomic except for copy construction and assignment and replace
them with global functions that can operate on the atomic type. atomic
has a number of global functions that can operate on std::atomic types
http://en.cppreference.com/w/cpp/atomic and we can look into implementing
similar ones (for example, mozilla::AtomicFetchAdd,
mozilla::AtomicExchange, etc. -- names to be bikeshedded outside of this
thread!)
There is an extensive discussion on bug 987887 about this. The proponents
of this change have argued that it makes it easier to spot that a given
type is an atomic one because the type is explicitly treated differently
than normal built-in types, even at the lack of a bit of convenience that
the member operators provide. The opponents of this change have argued
that the alternative is just as error prone as what we have today, and that
it diverges us from being compatible with the std::atomic type (in answer
to which I have said that is a good thing, since std::atomic is affected by
the same problem and I think is a bad idea for us to copy it.)
I am of course biased towards my side of the argument, so please read the
bug to get a better understanding of people's positions.
What do people feel about my proposal? Do you think it improves writing
and reviewing thread safe code to be less error prone?
Cheers,
--
Ehsan
http://ehsanakhgari.org/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: MOZ_ASSUME_UNREACHABLE is being misused
Karl Tomlinson writes: Jason Orendorff writes: If the code is truly unreachable, it doesn't matter what we replace it with. The question is what to do when the impossible occurs. To me, letting control flow past such a place is just as scary as undefined behavior. That depends on the particular code. Often enough, letting control flow past is relatively harmless compared to a crash. So I think our replacement for MOZ_ASSUME_UNREACHABLE should crash even in non-debug builds. This suggests MOZ_CRASH. Jason and I spoke on irc and realised that we were talking about 2 slightly different things. I apologize for taking a bit of a tangent to this particular sub thread. Given the history of MOZ_ASSUME_UNREACHABLE in JS code and the intention to remove MOZ_ASSUME_UNREACHABLE, MOZ_CRASH is the appropriate replacement for those existing use cases. However, I would like to discourage use of MOZ_CRASH in future code unless failure to match cases in a switch is really more scary than crashing. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: MOZ_ASSUME_UNREACHABLE is being misused
On 4/1/14, 4:37 PM, Karl Tomlinson wrote: Jason and I spoke on irc and realised that we were talking about 2 slightly different things. Yep. Filed bug 990764. If someone wants to take, we can continue discussion there. -j ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: MOZ_ASSUME_UNREACHABLE is being misused
On 4/1/14 5:37 PM, Karl Tomlinson wrote: Karl Tomlinson writes: However, I would like to discourage use of MOZ_CRASH in future code unless failure to match cases in a switch is really more scary than crashing. I think in general we should be willing to make more of our assertions fatal in release builds, especially in non-performance-sensitive code. Of course the choice of assertion depends on the characteristics of what's being asserted. --BDS ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::NullptrT on B2G ICS vs. everything else (incl. B2G JB and Android ICS)
On 04/01/2014 09:44 AM, Trevor Saunders wrote: b2g jb using gcc 4.7 was what I guessed as well, but http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-hamachi/1396314004/b2g_mozilla-central_hamachi_dep-bm72-build1-build10.txt.gz seems to provide some evidence to the contrary. configure says it finds gcc 4.4, but b2g 'helpfully' runs make with -s so we can't know what actually happens... Judging by the tinderbox behavior I've observed with nullptr stuff in the past -- including patches that dumped the compiler version in tbpl logs and then aborted the build -- I think the compiler on tinderbox for at least some kinds of b2g stuff is some weird amalgam that doesn't map to the gcc version numbering the world uses, with some backported patches for various things, or something. Which seems crazy, but everything I hear says attempting to seriously complain and get us moved to a modern gcc there is useless, so I haven't bothered. Jeff ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
I've got some outside experience with std::atomic so make of it what you
will :)
How often are you touching atomic variables directly? In my experience with
a similar thread safe inline ref count and smart pointer system to
Mozilla's (using std::atomic for the ref count), there's been no confusion
as the equivalent ref counted base class is super small and easy to
read/verify. In the other cases where std::atomic occurs, careful
consideration is already given to threading due to the nature of the code
so the similarity to a non-atomic integer type actually has prompted me to
have a (temporary) false sense of thread unsafety, rather than a false
sense of thread safety. And I think that is fine. For my team's code, I
think it's fine that they have similar notation as it makes operations
(like statistics counters) fairly easy to read vs explicit
atomicIncrement(counter, value) calls.
-Rob
On Tue, Apr 1, 2014 at 2:32 PM, Ehsan Akhgari ehsan.akhg...@gmail.comwrote:
The subject of this post is intentionally chosen to make you want to read
this. :-)
The summary is that I think the mozilla::Atomic API which is modeled after
std::atomic is harmful in that it makes it very non-obvious that you're
dealing with atomic integers. Basically the existing interface makes
mozilla::Atomic look like a normal integer, so that if you see code like:
--mRefCnt;
if (mRefCnt == 0) {
delete this;
}
You won't immediately think about checking the type of mRefCnt (the
refcount case being just an example here of course), which makes it hard to
spot that there is a thread safety bug in this code. Given that I and
three experienced Gecko hackers fell prey to this issue (bug 985878 and bug
987667), I thought about the underlying problem a bit, and managed to
convince myself that it's a bad idea for us to pretend that atomic integers
can be treated the same way as non-atomic integers.
So, over in bug 987887 I'm proposing to remove all of the methods on
mozilla::Atomic except for copy construction and assignment and replace
them with global functions that can operate on the atomic type. atomic
has a number of global functions that can operate on std::atomic types
http://en.cppreference.com/w/cpp/atomic and we can look into implementing
similar ones (for example, mozilla::AtomicFetchAdd,
mozilla::AtomicExchange, etc. -- names to be bikeshedded outside of this
thread!)
There is an extensive discussion on bug 987887 about this. The proponents
of this change have argued that it makes it easier to spot that a given
type is an atomic one because the type is explicitly treated differently
than normal built-in types, even at the lack of a bit of convenience that
the member operators provide. The opponents of this change have argued
that the alternative is just as error prone as what we have today, and that
it diverges us from being compatible with the std::atomic type (in answer
to which I have said that is a good thing, since std::atomic is affected by
the same problem and I think is a bad idea for us to copy it.)
I am of course biased towards my side of the argument, so please read the
bug to get a better understanding of people's positions.
What do people feel about my proposal? Do you think it improves writing
and reviewing thread safe code to be less error prone?
Cheers,
--
Ehsan
http://ehsanakhgari.org/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: MOZ_ASSUME_UNREACHABLE is being misused
Benjamin Smedberg writes: I think in general we should be willing to make more of our assertions fatal in release builds, especially in non-performance-sensitive code. Of course the choice of assertion depends on the characteristics of what's being asserted. Sounds good. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 2014-04-01, at 15:15, Rob Arnold tell...@gmail.com wrote: the similarity to a non-atomic integer type actually has prompted me to have a (temporary) false sense of thread unsafety, rather than a false sense of thread safety. That was my impression too. I usually find that I have to chase after the definition to ensure that the variable is atomic, not the other way around. I’m used to Java’s primitives, which are explicit. Just don’t do both. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 04/01/2014 02:32 PM, Ehsan Akhgari wrote: What do people feel about my proposal? Do you think it improves writing and reviewing thread safe code to be less error prone? As I said in the bug, not particularly. I don't think you can program with atomics in any sort of brain-off way, and I don't think the boilerplate difference of += versus fetch-and-add or whatever really affects that. To the extent things should be done differently, it should be that *template* functions that deal with atomic/non-atomic versions of the same algorithm deserve extra review and special care, and perhaps even should be implemented twice, rather than sharing a single implementation. And I think the cases in question here are flavors of approximately a single issue, and we do not have a fundamental problem here to be solved by making the API more obtuse in practice. Jeff ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
2014-04-01 18:40 GMT-04:00 Jeff Walden jwalden+...@mit.edu: On 04/01/2014 02:32 PM, Ehsan Akhgari wrote: What do people feel about my proposal? Do you think it improves writing and reviewing thread safe code to be less error prone? As I said in the bug, not particularly. I don't think you can program with atomics in any sort of brain-off way, and I don't think the boilerplate difference of += versus fetch-and-add or whatever really affects that. To the extent things should be done differently, it should be that *template* functions that deal with atomic/non-atomic versions of the same algorithm deserve extra review and special care, and perhaps even should be implemented twice, rather than sharing a single implementation. And I think the cases in question here are flavors of approximately a single issue, and we do not have a fundamental problem here to be solved by making the API more obtuse in practice. How are we going to enforce (and ensure that future people enforce) that? (The part about functions that deal with atomic/non-atomic versions of the same algorithm deserve extra review and special care) ? I like Ehsan's proposal because, as far as I am concerned, explicit function names help me very well to remember to check atomic semantics; especially if we follow the standard atomic naming where the functions start by atomic_ , e.g. std::atomic_fetch_add. On the other hand, if the function name that we use for that is just operator + then it becomes very hard for me as a reviewer, because I have to remember checking everytime I see a + to check if the variables at hand are atomics. Benoit ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Enable -Wswitch-enum? [was Re: MOZ_ASSUME_UNREACHABLE is being misused]
On 4/1/14, 10:22 AM, Daniel Holbert wrote: So, we have on the order of ~4400 switch statements that would potentially need expanding to avoid tripping this warning. clang on OS X reports 1635 -Wswitch-enum warnings (switch on enum not handling all enum cases). gcc reports 1048 -Wswitch-default warnings (switch without default case). clang seems to silently ignore gcc's -Wswitch-default flag. chris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 2014-04-01, 6:59 PM, Benoit Jacob wrote: 2014-04-01 18:40 GMT-04:00 Jeff Walden jwalden+...@mit.edu: On 04/01/2014 02:32 PM, Ehsan Akhgari wrote: What do people feel about my proposal? Do you think it improves writing and reviewing thread safe code to be less error prone? As I said in the bug, not particularly. I don't think you can program with atomics in any sort of brain-off way, and I don't think the boilerplate difference of += versus fetch-and-add or whatever really affects that. To the extent things should be done differently, it should be that *template* functions that deal with atomic/non-atomic versions of the same algorithm deserve extra review and special care, and perhaps even should be implemented twice, rather than sharing a single implementation. And I think the cases in question here are flavors of approximately a single issue, and we do not have a fundamental problem here to be solved by making the API more obtuse in practice. How are we going to enforce (and ensure that future people enforce) that? (The part about functions that deal with atomic/non-atomic versions of the same algorithm deserve extra review and special care) ? My proposal would enforce that! I like Ehsan's proposal because, as far as I am concerned, explicit function names help me very well to remember to check atomic semantics; especially if we follow the standard atomic naming where the functions start by atomic_ , e.g. std::atomic_fetch_add. On the other hand, if the function name that we use for that is just operator + then it becomes very hard for me as a reviewer, because I have to remember checking everytime I see a + to check if the variables at hand are atomics. Just to clarify my position a bit more, I think criticizing my position by pretending that I'm advocating for a brain-off way of programming with atomics is a bit contrived. I definitely understand that atomics require special feeding and care. What's under debate is whether we should make that obvious to authors and reviewers by not conflating things such as operator++ etc. to work on both atomic and non-atomic types. Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 2014-04-01, at 16:17, Ehsan Akhgari ehsan.akhg...@gmail.com wrote: Just to clarify my position a bit more, I think criticizing my position by pretending that I'm advocating for a brain-off way of programming with atomics is a bit contrived. I definitely understand that atomics require special feeding and care. What's under debate is whether we should make that obvious to authors and reviewers by not conflating things such as operator++ etc. to work on both atomic and non-atomic types. I don’t think that the pushback is based on the fact that code using Atomicuint32_t is at least as thread safe as code using uint32_t. As is, someone reading code is likely to see threading errors that are actually safe due to use of Atomic. The opposite - missing a real error - happens because we are human. It doesn’t seem more or less likely if you require the more explicit syntax. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 2014-04-01, 6:15 PM, Rob Arnold wrote: I've got some outside experience with std::atomic so make of it what you will :) How often are you touching atomic variables directly? In my experience with a similar thread safe inline ref count and smart pointer system to Mozilla's (using std::atomic for the ref count), there's been no confusion as the equivalent ref counted base class is super small and easy to read/verify. I was hesitant to give the refcount example to avoid confusing people to think I'm just talking about refcounts, but I'm glad you got the point! In the case of that mRefCnt variable, I originally did not pay attention to the bug in the code because nothing about |mRefCnt| looked like it's an atomic variable. To be honest, I don't go and check the type of every single variable that I manipulate in my patches, in a lot of cases I rely on cues in the surrounding code to tell me something about the type (unconsciously thinking, this thing is called mRefCnt, and it has operator++ and --, so it's got to be an integer.) I cannot speak why the reviewers did not catch this bug, but I suspect because of similar reasons. In the other cases where std::atomic occurs, careful consideration is already given to threading due to the nature of the code so the similarity to a non-atomic integer type actually has prompted me to have a (temporary) false sense of thread unsafety, rather than a false sense of thread safety. And I think that is fine. For my team's code, I think it's fine that they have similar notation as it makes operations (like statistics counters) fairly easy to read vs explicit atomicIncrement(counter, value) calls. I agree that in code which does obvious threading, my proposal won't improve things, but I'm mostly worried about bugs creeping in where we use atomics for things that are not obviously threading related. Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::NullptrT on B2G ICS vs. everything else (incl. B2G JB and Android ICS)
On 2014-04-01, 6:03 PM, Jeff Walden wrote: On 04/01/2014 09:44 AM, Trevor Saunders wrote: b2g jb using gcc 4.7 was what I guessed as well, but http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-hamachi/1396314004/b2g_mozilla-central_hamachi_dep-bm72-build1-build10.txt.gz seems to provide some evidence to the contrary. configure says it finds gcc 4.4, but b2g 'helpfully' runs make with -s so we can't know what actually happens... Judging by the tinderbox behavior I've observed with nullptr stuff in the past -- including patches that dumped the compiler version in tbpl logs and then aborted the build -- I think the compiler on tinderbox for at least some kinds of b2g stuff is some weird amalgam that doesn't map to the gcc version numbering the world uses, with some backported patches for various things, or something. Yeah, I have the same impression. Which seems crazy, but everything I hear says attempting to seriously complain and get us moved to a modern gcc there is useless, so I haven't bothered. Actually there is a chance for us to switch to a more modern (NDK) gcc in 1.5, but of course that will not affect the toolchain used for ICS as long as we need to support that. Cheers, Ehsan ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 2014-04-01, 7:32 PM, Martin Thomson wrote:
On 2014-04-01, at 16:17, Ehsan Akhgari ehsan.akhg...@gmail.com wrote:
Just to clarify my position a bit more, I think criticizing my position by
pretending that I'm advocating for a brain-off way of programming with atomics
is a bit contrived. I definitely understand that atomics require special
feeding and care. What's under debate is whether we should make that obvious
to authors and reviewers by not conflating things such as operator++ etc. to
work on both atomic and non-atomic types.
I don’t think that the pushback is based on the fact that code using
Atomicuint32_t is at least as thread safe as code using uint32_t.
As is, someone reading code is likely to see threading errors that are actually safe
due to use of Atomic. The opposite - missing a real error - happens because
we are human. It doesn’t seem more or less likely if you require the more explicit
syntax.
So are you suggesting that if the code in my original patch looked like
this:
// (a)
if (mozilla::AtomicFetchSub(mRefCnt, -1) == 1) {
delete this;
}
I would still go ahead and rewrite it as:
// (b)
mozilla::AtomicFetchSub(mRefCnt, -1);
if (mozilla::AtomicLoad(mRefCnt) == 0) {
delete this;
}
My contention is that it is obvious enough by looking at (a) to tell
that mRefCnt is an atomic value which should be handled with the
necessary care, so the pattern (b) would be caught either at code
writing time or at code review time.
Cheers,
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On Tue, Apr 01, 2014 at 05:32:12PM -0400, Ehsan Akhgari wrote:
The subject of this post is intentionally chosen to make you want to read
this. :-)
The summary is that I think the mozilla::Atomic API which is modeled after
std::atomic is harmful in that it makes it very non-obvious that you're
dealing with atomic integers. Basically the existing interface makes
mozilla::Atomic look like a normal integer, so that if you see code like:
--mRefCnt;
if (mRefCnt == 0) {
delete this;
}
You won't immediately think about checking the type of mRefCnt (the
refcount case being just an example here of course), which makes it hard to
spot that there is a thread safety bug in this code.
Actually, the thread safety bug in this code is largely the same whether the
type of mRefCnt is mozilla::Atomic or not. Compiler optimizations may
remove the bug, but it's there to begin with.
As I said in the bug, all this is saying is that thread safety is hard,
and atomics are merely one of the tools to achieve thread safety. They
are not a magic wand that fixes thread safety.
I also think that making their API not have operators like std::atomic
has would bring a false sense of security to people writing code using
them, because it would supposedly be less confusing when it really
wouldn't.
Mike
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 2014-04-01, at 16:48, Ehsan Akhgari ehsan.akhg...@gmail.com wrote:
My contention is that it is obvious enough by looking at (a) to tell that
mRefCnt is an atomic value which should be handled with the necessary care,
so the pattern (b) would be caught either at code writing time or at code
review time.
My point was that:
--mRefCnt;
if (mRefCnt == 0) {
delete this;
}
is as much an obvious threading issue as:
if (--mRefCnt == 0) {
delete this;
}
…absent some extra knowledge.
More verbosity isn’t going to change this. This might:
count_type tmp = --mRefCnt;
if (tmp == 0) {
delete this;
}
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 2014-04-01, 7:58 PM, Mike Hommey wrote:
On Tue, Apr 01, 2014 at 05:32:12PM -0400, Ehsan Akhgari wrote:
The subject of this post is intentionally chosen to make you want to read
this. :-)
The summary is that I think the mozilla::Atomic API which is modeled after
std::atomic is harmful in that it makes it very non-obvious that you're
dealing with atomic integers. Basically the existing interface makes
mozilla::Atomic look like a normal integer, so that if you see code like:
--mRefCnt;
if (mRefCnt == 0) {
delete this;
}
You won't immediately think about checking the type of mRefCnt (the
refcount case being just an example here of course), which makes it hard to
spot that there is a thread safety bug in this code.
Actually, the thread safety bug in this code is largely the same whether the
type of mRefCnt is mozilla::Atomic or not. Compiler optimizations may
remove the bug, but it's there to begin with.
That is the code after I changed it. :-) Here is the original patch
which introduced this bug:
https://bug935778.bugzilla.mozilla.org/attachment.cgi?id=8385072 The
thread safety issue was _not_ there before that patch.
As I said in the bug, all this is saying is that thread safety is hard,
and atomics are merely one of the tools to achieve thread safety. They
are not a magic wand that fixes thread safety.
Did you read https://bugzilla.mozilla.org/show_bug.cgi?id=987887#c20?
Continuing to reduce my argument here to atomics should be a magic
wand is really depressing.
I also think that making their API not have operators like std::atomic
has would bring a false sense of security to people writing code using
them, because it would supposedly be less confusing when it really
wouldn't.
Can you please give an example of that concern? I'm not sure if I follow.
Thanks,
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On Tue, Apr 01, 2014 at 08:34:01PM -0400, Ehsan Akhgari wrote:
On 2014-04-01, 7:58 PM, Mike Hommey wrote:
On Tue, Apr 01, 2014 at 05:32:12PM -0400, Ehsan Akhgari wrote:
The subject of this post is intentionally chosen to make you want to read
this. :-)
The summary is that I think the mozilla::Atomic API which is modeled after
std::atomic is harmful in that it makes it very non-obvious that you're
dealing with atomic integers. Basically the existing interface makes
mozilla::Atomic look like a normal integer, so that if you see code like:
--mRefCnt;
if (mRefCnt == 0) {
delete this;
}
You won't immediately think about checking the type of mRefCnt (the
refcount case being just an example here of course), which makes it hard to
spot that there is a thread safety bug in this code.
Actually, the thread safety bug in this code is largely the same whether the
type of mRefCnt is mozilla::Atomic or not. Compiler optimizations may
remove the bug, but it's there to begin with.
That is the code after I changed it. :-) Here is the original patch which
introduced this bug:
https://bug935778.bugzilla.mozilla.org/attachment.cgi?id=8385072 The
thread safety issue was _not_ there before that patch.
I'm not saying there was a thread safety issue before. I'm saying that in
your example, there is largely the same thread safety issue whether the
code uses a plain int or an atomic one. The use of atomics is _not_ hiding
this bug.
As I said in the bug, all this is saying is that thread safety is hard,
and atomics are merely one of the tools to achieve thread safety. They
are not a magic wand that fixes thread safety.
Did you read https://bugzilla.mozilla.org/show_bug.cgi?id=987887#c20?
Did you read my answer to that comment?
Continuing to reduce my argument here to atomics should be a magic wand is
really depressing.
I also think that making their API not have operators like std::atomic
has would bring a false sense of security to people writing code using
them, because it would supposedly be less confusing when it really
wouldn't.
Can you please give an example of that concern? I'm not sure if I follow.
Just take the original patch which introduced the bug. I seriously don't
believe if the patch had been using FetchAndAdd instead of ++/-- the issue
would have had more chance of being caught at review. And I don't
believe it would make people more likely to make atomic-using code
thread safe.
Back to Kyle's comment from the bug, the core issue is that we don't
have anything that tells us that $function is expected to be called from
different threads at the same time, and that as such it _needs_ to be
thread safe or reentrant in some way. Maybe we need to invent those
markers, and to have ways to test when they are missing (like special
builds that ensure that functions without those markers are never called
from two threads simultaneously).
Mike
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On Wednesday 2014-04-02 12:50 +0900, Mike Hommey wrote: On Tue, Apr 01, 2014 at 08:34:01PM -0400, Ehsan Akhgari wrote: That is the code after I changed it. :-) Here is the original patch which introduced this bug: https://bug935778.bugzilla.mozilla.org/attachment.cgi?id=8385072 The thread safety issue was _not_ there before that patch. I'm not saying there was a thread safety issue before. I'm saying that in your example, there is largely the same thread safety issue whether the code uses a plain int or an atomic one. The use of atomics is _not_ hiding this bug. That's not the issue. The issue is that the syntax is hiding the use of atomics. And whether the code would have had a thread-safety bug if it hadn't been using atomics is also not relevant; lots of single-threaded code has thread-safety bugs if you magically decide to violate its threading invariants and use the same object on multiple threads when it wasn't designed for that. The issue here is whether this particular way of writing threadsafe code leads people modifying that code to make mistakes because they don't even notice that it's threadsafe code. As I said in the bug, all this is saying is that thread safety is hard, and atomics are merely one of the tools to achieve thread safety. They are not a magic wand that fixes thread safety. Did you read https://bugzilla.mozilla.org/show_bug.cgi?id=987887#c20? Did you read my answer to that comment? I think you're continuing to ignore the fact that using operator overloading obscures what those operators are doing underneath, and when that difference is important to the reader of the code, overloading might not be a good idea. Continuing to reduce my argument here to atomics should be a magic wand is really depressing. I also think that making their API not have operators like std::atomic has would bring a false sense of security to people writing code using them, because it would supposedly be less confusing when it really wouldn't. Can you please give an example of that concern? I'm not sure if I follow. Just take the original patch which introduced the bug. I seriously don't believe if the patch had been using FetchAndAdd instead of ++/-- the issue would have had more chance of being caught at review. And I don't believe it would make people more likely to make atomic-using code thread safe. You might claim that the increased chance of being caught is small, but claiming it's zero is laughable. -David -- 턞 L. David Baron http://dbaron.org/ 턂 턢 Mozilla https://www.mozilla.org/ 턂 Before I built a wall I'd ask to know What I was walling in or walling out, And to whom I was like to give offense. - Robert Frost, Mending Wall (1914) signature.asc Description: Digital signature ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On 4/1/2014 4:32 PM, Ehsan Akhgari wrote: So, over in bug 987887 I'm proposing to remove all of the methods on mozilla::Atomic except for copy construction and assignment and replace them with global functions that can operate on the atomic type. atomic has a number of global functions that can operate on std::atomic types http://en.cppreference.com/w/cpp/atomic and we can look into implementing similar ones (for example, mozilla::AtomicFetchAdd, mozilla::AtomicExchange, etc. -- names to be bikeshedded outside of this thread!) I am very much against this, and I think your proposal is a medicine which is far more harmful than the problem ever is or was. My original design for mozilla::Atomic was meant to avoid what I saw as the biggest footgun: you cannot misuse mozilla::Atomic in such a way that you combine atomic and non-atomic accesses on a single variable. You also cannot mix-and-match different memory orders on the same atomic variable (in this manner, I willfully departed from std::atomic). Using global functions instead would tend to cause people to want to draw this information from the point of declaration to the point of use: note that it is much easier in tooling to find the declaration of a variable than it is to find all uses of one. There are other issues with your design. The verbose names have, to me at least, been a constant source of confusion: I can never recall if FetchAndAdd returns the value prior to or subsequent to the addition; note that operator+= does not have the same problems. As for the original problem, I think you're misdiagnosing the failure. The only real problem of the code is that it's possible that people could mistake the variable for not being an atomic variable. Taking a look through most of the uses of atomics in our codebase, the cases where people are liable to not realize that these are atomics are relatively few, especially if patch authors and reviewers are both well-acquainted with the surrounding code. So it's not clear to me that including extra-verbose names would really provide much of a benefit. Instead, I think you're overreacting to what is a comparatively rare case: a method templated to support both non-thread-safe and thread-safe variants, and much less frequently used or tested in thread-safe conditions (note that if you made the same mistake with nsISupports-based refcounting, your patch would be fail sufficiently many tests to be backed out of mozilla-inbound immediately). -- Joshua Cranmer Thunderbird and DXR developer Source code archæologist ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On Wed, Apr 2, 2014 at 12:12 PM, L. David Baron dba...@dbaron.org wrote: The issue here is whether this particular way of writing threadsafe code leads people modifying that code to make mistakes because they don't even notice that it's threadsafe code. I completely agree. And because using the current Atomic code obscures the need to be concerned about threadsafety, I strongly support Ehsan's proposal. As I said in the bug, all this is saying is that thread safety is hard, and atomics are merely one of the tools to achieve thread safety. They are not a magic wand that fixes thread safety. Did you read https://bugzilla.mozilla.org/show_bug.cgi?id=987887#c20? Did you read my answer to that comment? I think you're continuing to ignore the fact that using operator overloading obscures what those operators are doing underneath, and when that difference is important to the reader of the code, overloading might not be a good idea. +Infinity - Kyle ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: mozilla::Atomic considered harmful
On Wed, Apr 2, 2014 at 12:11 AM, Joshua Cranmer pidgeo...@gmail.comwrote: My original design for mozilla::Atomic was meant to avoid what I saw as the biggest footgun: you cannot misuse mozilla::Atomic in such a way that you combine atomic and non-atomic accesses on a single variable. You also cannot mix-and-match different memory orders on the same atomic variable (in this manner, I willfully departed from std::atomic). Using global functions instead would tend to cause people to want to draw this information from the point of declaration to the point of use: note that it is much easier in tooling to find the declaration of a variable than it is to find all uses of one. AFAICT Ehsan is proposing that we continue using atomic types, and his explicit functions would only operate on those atomic types, so his proposal would preserve this aspect of your design. I'm in favor of his proposal. Rob -- Jtehsauts tshaei dS,o n Wohfy Mdaon yhoaus eanuttehrotraiitny eovni le atrhtohu gthot sf oirng iyvoeu rs ihnesa.rt sS?o Whhei csha iids teoa stiheer :p atroa lsyazye,d 'mYaonu,r sGients uapr,e tfaokreg iyvoeunr, 'm aotr atnod sgaoy ,h o'mGee.t uTph eann dt hwea lmka'n? gBoutt uIp waanndt wyeonut thoo mken.o w ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Firefox build (Nightly) thread with root permission
Hi, When I added code with a function udev_monitor_new_from_socket(), the Nightly stops executing everything while this function returns an error fd. I strongly suspect this happens due to the permission issue while Nightly does not allow me to gain root privileges. However, this is necessary for me to complete my experiment. How can I obtain a decent permission while I do this? Thanks ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
