Re: Enable -Wswitch-enum? [was Re: MOZ_ASSUME_UNREACHABLE is being misused]
(2014/04/07 14:27), Karl Tomlinson wrote:
It is allowed in N3242. I think the relevant sections are
5.2.9 Static cast
Thank you for the pointer.
I found a floating copy of n3242.pdf at the following url.
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3242.pdf
I think 7.2 10 is also relevant here.
--- quote ---
An expression of arithmetic or enumeration type can be converted to an
enumeration type explicitly. The
value is unchanged if it is in the range of enumeration values of the
enumeration type; otherwise the resulting
enumeration value is unspecified.
--- end quote
I take so :
typedef enum { a = 1, b, c = 10 } T;
T x;
x = 3; /* is OK it is within [1..10] although 3 is not in the list
explicitly */
but
x = 32; /*`unspecified' because it is outside [1..2^16] */
(I read the specification to mean
that the range of enumeration values is
[0 or 1 .. maximum_necessary_for_the_declared_maximum_value]
where maximum_necessary_for_the_declared_maximum_value is
the 2^M or (2^M-1) etc. depending on how the
negative value is represented. 2's complement or 1's complement.)
The description is very complex, but
it seems that the enumeration range be
calculated to produce the narrowest bit field that can contain
all the explicitly declared values (min .. max)
This is the range of values.
Anyway, in my example above, a compiler can do anything
if x = 32 is executed (?).
Hmm.I will re-read 7.2.7.
TIA
___
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]
chiaki ISHIKAWA writes:
I think 7.2 10 is also relevant here.
--- quote ---
An expression of arithmetic or enumeration type can be converted
to an enumeration type explicitly. The
value is unchanged if it is in the range of enumeration values of
the enumeration type; otherwise the resulting
enumeration value is unspecified.
--- end quote
I take so :
typedef enum { a = 1, b, c = 10 } T;
T x;
Anyway, in my example above, a compiler can do anything
if x = 32 is executed (?).
Note here the enumeration value is unspecified, which I assume
merely means the compiler can choose anything for the value of x.
That might be considerably safer than undefined behavior of the
program.
___
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 2014-04-07 6:00 AM, Karl Tomlinson wrote:
chiaki ISHIKAWA writes:
I think 7.2 10 is also relevant here.
--- quote ---
An expression of arithmetic or enumeration type can be converted
to an enumeration type explicitly. The
value is unchanged if it is in the range of enumeration values of
the enumeration type; otherwise the resulting
enumeration value is unspecified.
--- end quote
I take so :
typedef enum { a = 1, b, c = 10 } T;
T x;
Anyway, in my example above, a compiler can do anything
if x = 32 is executed (?).
Note here the enumeration value is unspecified, which I assume
merely means the compiler can choose anything for the value of x.
That might be considerably safer than undefined behavior of the
program.
Right. The intention here is that the compiler is allowed to pick some
underlying integer type, which must be able to represent all declared
values of the enumeration, and then silently accept in-range values and
truncate out-of-range values *for that type* -- but it's *not* allowed
to apply assume the programmer never does that optimizations as it
would for undefined behavior.
I don't know what the C++ committee's attitude was, but the C committee
specifically wanted to allow use of `enum` to declare bitmasks:
enum X {
X_FOO = 0x0001,
X_BAR = 0x0002,
/* ... exhaustive list of bit flags here ... */
X_BLURF = 0x8000
};
For that use case, any _combination_ of X_* flags must be representable
by the underlying integer type; note that many such combinations are
outside the *declared* range of values.
zw
___
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 Fri, 04 Apr 2014 11:03:57 -0400, Zack Weinberg wrote: On Tue, Apr 1, 2014 at 10:13 PM, Karl Tomlinson mozn...@karlt.net wrote: Does WARNINGS_AS_ERRORS make the default:MOZ_CRASH() unnecessary? No, because it's possible that the thing you're testing is not actually a valid enum value, such as if it was incorrectly cast. Our codebase has no shortage of dubious casts. Also, in many cases default:MOZ_CRASH() would kill control flow paths that make the compiler think variables might be used uninitialized. OK. Even with -Wswitch issuing no warning with no default clause, the compiler may need to handle the switch matching no case, because enumeration types may hold values that don't match any of their enumerator values. However, if the compiler knows that falling through would cause a variable to be used uninitialized, then it knows that would have undefined behaviour, so it would not need to handle that situation in any particular way. ___ 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]
(2014/04/07 10:16), Karl Tomlinson wrote: because enumeration types may hold values that don't match any of their enumerator values. Is this allowed by C (or C++) specification today? [Yes, I know the compiler in the past did not care much.] I thought the stricter warnings of compilers today is that if something like that happens, anything goes in principle (because of the spec allows the compiler to produce such code.) Maybe I was mistaken. TIA ___ 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]
chiaki ISHIKAWA writes: (2014/04/07 10:16), Karl Tomlinson wrote: because enumeration types may hold values that don't match any of their enumerator values. Is this allowed by C (or C++) specification today? It is allowed in N3242. I think the relevant sections are 5.2.9 Static cast 10 A value of integral or enumeration type can be explicitly converted to an enumeration type. The value is unchanged if the original value is within the range of the enumeration values (7.2). Otherwise, the resulting enumeration value is unspecified. 7.2 Enumeration declarations 7 For an enumeration whose underlying type is fixed, the values of the enumeration are the values of the underlying type. Otherwise, for an enumeration where e min is the smallest enumerator and e max is the largest, the values of the enumeration are the values in the range b min to b max , defined as follows: Let K be 1 for a two’s complement representation and 0 for a one’s complement or sign-magnitude representation. b max is the smallest value greater than or equal to max(|e min | − K, |e max |) and equal to 2 M − 1, where M is a non-negative integer. b min is zero if e min is non-negative and −(b max + K) otherwise. The size of the smallest bit-field large enough to hold all the values of the enumeration type is max(M, 1) if b min is zero and M + 1 otherwise. It is possible to define an enumeration that has values not defined by any of its enumerators. If the enumerator-list is empty, the values of the enumeration are as if the enumeration had a single enumerator with value 0. ___ 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]
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 04/02/2014 07:37 AM, Aryeh Gregor wrote: On Tue, Apr 1, 2014 at 5:56 PM, Zack Weinberg za...@panix.com 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. If there are few enough that this is sane. There are at least a few switches on nsresult in the tree, IIRC. Those would have to be rewritten as if/else. I think we have some other big enums lying around, although probably not so dramatic. Given the numbers posted elsewhere, I think this is clearly not going anywhere until someone has time and working development machine to find out just how burdensome it is. I have no idea how long it will be before I personally can do such experiments, alas. On Tue, Apr 1, 2014 at 10:13 PM, Karl Tomlinson mozn...@karlt.net wrote: Does WARNINGS_AS_ERRORS make the default:MOZ_CRASH() unnecessary? No, because it's possible that the thing you're testing is not actually a valid enum value, such as if it was incorrectly cast. Our codebase has no shortage of dubious casts. Also, in many cases default:MOZ_CRASH() would kill control flow paths that make the compiler think variables might be used uninitialized. zw -BEGIN PGP SIGNATURE- Version: GnuPG v1 Comment: Using GnuPG with Icedove - http://www.enigmail.net/ iQIcBAEBCAAGBQJTPsnUAAoJEJH8wytnaapkyRgQALgWSAFa+FLLyYuFeuQis9zs KhqmNCYmCtTk9KNv8WhPTZCN6QYEekKPGKyoi5A+DXaRPqkqf6N2X3F8bWQbIrTB hZeRKm7Ks9TEvrBWQy8NwGV0Q2BftWpvvoKlHu44/Z8A1o8bW9X3kA/4GRltCi6+ jizXprEQ96opIOA2/7X7JdOG4jMnyNgeX236cgTQOIzf1RF2at70OGfVOZ+FcnPO UjctNx32L6xd306SIzWI/gwGXP7e9rxVl87/vjk+R8HcBoNNftkphRoZuP1L7h9E QNAAwdYUUa5TWrku9jeYIKgzI+gFTt2IjTsglIx3tHSlhB9Eigni6FzqYPjmUjQ+ sRn+726/cVDu8o9iKyZ3vTAxEYHHGFQVTLozlHp3zaVjqu02k6QvCZ4qkBJxU+0D zr3QZFg4WgN0FnS15b8HeeJKXfE1x8FQWsT7lVITlE7+kNq3DgHK7REpJvwxvGFX L6IdTY2Ag1V3ytywWgY8Qh1Tjvf73j+tMcO9Ka2auyGDJzwwem1BpfHJiCfQ4So4 SCJ/xH65p7Twgh7L//gIwMo2lhovP3/YTel5yVXSe5yIAzfoJ3mIMAKmF6vkcI55 ytD48TyNCxCCtEndc3QnsGA5EvRnM1tkAb01dfEVmrVmruFXoR6cTjJ3yiIgU3V3 OXTT6fwLxm/4bqUEwDcp =TW5u -END PGP 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 Tue, Apr 1, 2014 at 5:56 PM, Zack Weinberg za...@panix.com 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. If there are few enough that this is sane. There are at least a few switches on nsresult in the tree, IIRC. Those would have to be rewritten as if/else. I think we have some other big enums lying around, although probably not so dramatic. On Tue, Apr 1, 2014 at 10:13 PM, Karl Tomlinson mozn...@karlt.net wrote: Does WARNINGS_AS_ERRORS make the default:MOZ_CRASH() unnecessary? No, because it's possible that the thing you're testing is not actually a valid enum value, such as if it was incorrectly cast. Our codebase has no shortage of dubious casts. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: MOZ_ASSUME_UNREACHABLE is being misused
== 3) result = y*y*y;
else if (x == 4) result = (y*y + 1) / (y + 10);
else if (x == 5) result = y*y*y + y;
else if (x == 6) result = 2*y;
else if (x == 7) result = y*y + 3*y;
else if (x == 8) result = 5*y*y*y + y*y;
else if (x == 9) result = 7*y*y + 1;
else if (x == 10) result = 3*y*y*y - y + 1;
else {
UNREACHABLE();
}
return result;
}
Results:
Clang3.4 implements this using a jump table and does the range check
even with unreachable. Unreachable still has the effect of merging the
last case with the default case, which allows to generate slightly
smaller code.
GCC4.6 implements this as a dump chain of conditional jumps;
unreachable has the effect of jumping to the end of the function
without a 'ret' instruction, i.e. continuing execution outside of the
function! (I actually verified that that's what happens in GDB).
Conclusion: if we hit the 'unreachable' path, with clang3.4 we're
safe, but with gcc4.6 we end up running code of unrelated functions,
typically crashing, very possibly doing exploitable things first!
*
*
Test program 3/4 - c.cpp
If-branch on a condition already known to be never met due to an
earlier unreachable statement.
Source code:
#ifdef USE_UNREACHABLE
#define UNREACHABLE() __builtin_unreachable()
#else
#define UNREACHABLE()
#endif
unsigned int foo(unsigned int x)
{
bool b = x 100;
if (b) {
UNREACHABLE();
}
if (b) {
return (x*x*x*x + x*x + 1) / (x*x*x + x + 1234);
}
return x;
}
Results:
Clang3.4: unreachable has no effect.
GCC4.6: the unreachable statement is fully understood to make this
condition never met, and GCC4.6 uses this to omit it entirely.
Without unreachable:
.cfi_startproc
cmpl$100, %edi
movl%edi, %eax
jbe .L2
movl%edi, %ecx
xorl%edx, %edx
imull %edi, %ecx
addl$1, %ecx
imull %edi, %ecx
imull %ecx, %eax
addl$1234, %ecx
addl$1, %eax
divl%ecx
.L2:
rep
ret
.cfi_endproc
With unreachable:
.cfi_startproc
movl%edi, %eax
ret
.cfi_endproc
*
*
Test program 4/4 - d.cpp
If-branch on a condition already known to be always met due to an
earlier unreachable statement on the opposite condition.
Source code:
#ifdef USE_UNREACHABLE
#define UNREACHABLE() __builtin_unreachable()
#else
#define UNREACHABLE()
#endif
unsigned int foo(unsigned int x)
{
bool b = x 100;
if (!b) {
UNREACHABLE();
}
if (b) {
return (x*x*x*x + x*x + 1) / (x*x*x + x + 1234);
}
return x;
}
Clang3.4: unreachable has no effect.
GCC4.6: the unreachable statement is fully understood to make this
condition always met, and GCC4.6 uses this to remove the conditional
branch and unconditionally take this branch.
Without unreachable:
.cfi_startproc
cmpl$100, %edi
movl%edi, %eax
jbe .L2
movl%edi, %ecx
xorl%edx, %edx
imull %edi, %ecx
addl$1, %ecx
imull %edi, %ecx
imull %ecx, %eax
addl$1234, %ecx
addl$1, %eax
divl%ecx
.L2:
rep
ret
.cfi_endproc
With unreachable:
.cfi_startproc
movl%edi, %ecx
xorl%edx, %edx
imull %edi, %ecx
addl$1, %ecx
imull %edi, %ecx
movl%ecx, %eax
addl$1234, %ecx
imull %edi, %eax
addl$1, %eax
divl%ecx
ret
.cfi_endproc
2014-03-28 12:25 GMT-04:00 Benoit Jacob jacob.benoi...@gmail.com:
Hi,
Despite a helpful, scary comment above its definition in
mfbt/Assertions.h, MOZ_ASSUME_UNREACHABLE is being misused. Not pointing
fingers to anything specific here, but see
http://dxr.mozilla.org/mozilla-central/search?q=MOZ_ASSUME_UNREACHABLEcase=true.
The only reason why one might want an unreachability marker instead of
simply doing nothing, is as an optimization --- a rather arcane, dangerous,
I-know-what-I-am-doing kind of optimization.
How can we help people not misuse?
Should we rename it to something more explicit about what it is doing,
such as perhaps MOZ_UNREACHABLE_UNDEFINED_BEHAVIOR ?
Should we give typical code a macro that does what they want and sounds
like what they want? Really, what typical code wants is a no-operation
instead of undefined-behavior; now, that is exactly the same as
MOZ_ASSERT(false, error). Maybe this syntax is unnecessarily annoying,
and it would be worth adding a macro for that, i.e
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
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
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: 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: 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: 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: 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: 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: MOZ_ASSUME_UNREACHABLE is being misused
Joshua Cranmer wrote: MOZ_ASSUME_UNREACHABLE_MARKER tells the compiler that this code and everything after it can't be reached, so it need do anything. Clang will delete the code after this branch and decide to not emit any control flow. I have found this to be unhelpful when debugging because you can't jump into deleted code. -- Warning: May contain traces of nuts. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: MOZ_ASSUME_UNREACHABLE is being misused
On 3/28/14, 7:03 PM, Joshua Cranmer wrote: I included MOZ_ASSUME_UNREACHABLE_MARKER because that macro is the compiler-specific optimize me intrinsic, which I believe was the whole point of the original MOZ_ASSUME_UNREACHABLE. AFAIU, MOZ_ASSUME_UNREACHABLE_MARKER crashes on all Gecko platforms, but I included MOZ_CRASH to ensure the behavior was consistent for all platforms. No, MOZ_ASSUME_UNREACHABLE_MARKER tells the compiler that this code and everything after it can't be reached, so it need do anything. Clang will delete the code after this branch and decide to not emit any control flow. It may crash, but this is in the same vein that reading an uninitialized variable may crash: it can certainly do a lot of wrong and potentially exploitable things first. So what is an example of an appropriate use of MOZ_ASSUME_UNREACHABLE in Gecko today? If we can identify a good example, then perhaps we can create an easier to use solution. If we have no good examples, then we should probably remove this tricky macro. chris ___ 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-31 15:22 GMT-04:00 Chris Peterson cpeter...@mozilla.com: On 3/28/14, 7:03 PM, Joshua Cranmer wrote: I included MOZ_ASSUME_UNREACHABLE_MARKER because that macro is the compiler-specific optimize me intrinsic, which I believe was the whole point of the original MOZ_ASSUME_UNREACHABLE. AFAIU, MOZ_ASSUME_UNREACHABLE_MARKER crashes on all Gecko platforms, but I included MOZ_CRASH to ensure the behavior was consistent for all platforms. No, MOZ_ASSUME_UNREACHABLE_MARKER tells the compiler that this code and everything after it can't be reached, so it need do anything. Clang will delete the code after this branch and decide to not emit any control flow. It may crash, but this is in the same vein that reading an uninitialized variable may crash: it can certainly do a lot of wrong and potentially exploitable things first. So what is an example of an appropriate use of MOZ_ASSUME_UNREACHABLE in Gecko today? That's a very good question to ask at this point! Good examples are examples where 1) it is totally guaranteed that the location is unreachable, and 2) the surrounding code is performance-critical for at least some caller. Example 1: Right *after* (not *before* !) a guaranteed crash in generic code, like this one: http://hg.mozilla.org/mozilla-central/file/df7b26e90378/build/annotationProcessors/CodeGenerator.java#l329 I'm not familiar with this code, but, being in a code generator, I can trust that this might be performance critical, and is really unreachable. Example 2: In the default case of a performance-critical switch statement that we have an excellent reason of thinking is completely unreachable. Example: http://hg.mozilla.org/mozilla-central/file/df7b26e90378/js/src/gc/RootMarking.cpp#l42 Again I'm not familiar with this code, but I can trust that it's performance-critical, and since that function is static to this cpp file, I can trust that the callers of this function are only a few local functions that are aware of the fact that it would be very dangerous to call this function with a bad 'kind' (though I wish that were said in a big scary warning). The UNREACHABLE here would typically allow the compiler to skip checking that 'kind' is in range before implementing this switch statement with a jump-table, so, if this code is performance-critical to the point that the cost of checking that 'kind' is in range is significant, then the UNREACHABLE here is useful. Benoit ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
MOZ_ASSUME_UNREACHABLE is being misused
Hi, Despite a helpful, scary comment above its definition in mfbt/Assertions.h, MOZ_ASSUME_UNREACHABLE is being misused. Not pointing fingers to anything specific here, but see http://dxr.mozilla.org/mozilla-central/search?q=MOZ_ASSUME_UNREACHABLEcase=true. The only reason why one might want an unreachability marker instead of simply doing nothing, is as an optimization --- a rather arcane, dangerous, I-know-what-I-am-doing kind of optimization. How can we help people not misuse? Should we rename it to something more explicit about what it is doing, such as perhaps MOZ_UNREACHABLE_UNDEFINED_BEHAVIOR ? Should we give typical code a macro that does what they want and sounds like what they want? Really, what typical code wants is a no-operation instead of undefined-behavior; now, that is exactly the same as MOZ_ASSERT(false, error). Maybe this syntax is unnecessarily annoying, and it would be worth adding a macro for that, i.e. similar to MOZ_CRASH but only affecting DEBUG builds? What would be a good name for it? Is it worth keeping a close analogy with the unreachable-marker macro to steer people away from it --- e.g. maybe MOZ_UNREACHABLE_NO_OPERATION or even just MOZ_UNREACHABLE? So that people couldn't miss it when they look for UNREACHABLE macros? 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 3/28/14, 12:25 PM, Benoit Jacob wrote:
Should we give typical code a macro that does what they want and sounds
like what they want? Really, what typical code wants is a no-operation
instead of undefined-behavior; now, that is exactly the same as
MOZ_ASSERT(false, error). Maybe this syntax is unnecessarily annoying,
and it would be worth adding a macro for that, i.e. similar to MOZ_CRASH
but only affecting DEBUG builds? What would be a good name for it? Is it
worth keeping a close analogy with the unreachable-marker macro to steer
people away from it --- e.g. maybe MOZ_UNREACHABLE_NO_OPERATION or even
just MOZ_UNREACHABLE? So that people couldn't miss it when they look for
UNREACHABLE macros?
How about replacing MOZ_ASSUME_UNREACHABLE with two new macros like:
#define MOZ_ASSERT_UNREACHABLE() \
MOZ_ASSERT(false, MOZ_ASSERT_UNREACHABLE)
#define MOZ_CRASH_UNREACHABLE() \
do { \
MOZ_ASSUME_UNREACHABLE_MARKER();\
MOZ_CRASH(MOZ_CRASH_UNREACHABLE); \
} while (0)
chris
___
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 13:23 GMT-04:00 Chris Peterson cpeter...@mozilla.com:
On 3/28/14, 12:25 PM, Benoit Jacob wrote:
Should we give typical code a macro that does what they want and sounds
like what they want? Really, what typical code wants is a no-operation
instead of undefined-behavior; now, that is exactly the same as
MOZ_ASSERT(false, error). Maybe this syntax is unnecessarily annoying,
and it would be worth adding a macro for that, i.e. similar to MOZ_CRASH
but only affecting DEBUG builds? What would be a good name for it? Is it
worth keeping a close analogy with the unreachable-marker macro to steer
people away from it --- e.g. maybe MOZ_UNREACHABLE_NO_OPERATION or even
just MOZ_UNREACHABLE? So that people couldn't miss it when they look for
UNREACHABLE macros?
How about replacing MOZ_ASSUME_UNREACHABLE with two new macros like:
#define MOZ_ASSERT_UNREACHABLE() \
MOZ_ASSERT(false, MOZ_ASSERT_UNREACHABLE)
#define MOZ_CRASH_UNREACHABLE() \
do { \
MOZ_ASSUME_UNREACHABLE_MARKER();\
MOZ_CRASH(MOZ_CRASH_UNREACHABLE); \
} while (0)
MOZ_ASSUME_UNREACHABLE_MARKER tells the compiler feel free to arbitrarily
miscompile this, and anything from that point on in this branch, as you may
assume that this code is unreachable. So it doesn't really serve any
purpose to add a MOZ_CRASH after a MOZ_ASSUME_UNREACHABLE_MARKER.
Benoit
chris
___
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
On Fri 28 Mar 2014 01:05:34 PM PDT, Benoit Jacob wrote:
2014-03-28 13:23 GMT-04:00 Chris Peterson cpeter...@mozilla.com:
On 3/28/14, 12:25 PM, Benoit Jacob wrote:
Should we give typical code a macro that does what they want and sounds
like what they want? Really, what typical code wants is a no-operation
instead of undefined-behavior; now, that is exactly the same as
MOZ_ASSERT(false, error). Maybe this syntax is unnecessarily annoying,
and it would be worth adding a macro for that, i.e. similar to MOZ_CRASH
but only affecting DEBUG builds? What would be a good name for it? Is it
worth keeping a close analogy with the unreachable-marker macro to steer
people away from it --- e.g. maybe MOZ_UNREACHABLE_NO_OPERATION or even
just MOZ_UNREACHABLE? So that people couldn't miss it when they look for
UNREACHABLE macros?
How about replacing MOZ_ASSUME_UNREACHABLE with two new macros like:
#define MOZ_ASSERT_UNREACHABLE() \
MOZ_ASSERT(false, MOZ_ASSERT_UNREACHABLE)
#define MOZ_CRASH_UNREACHABLE() \
do { \
MOZ_ASSUME_UNREACHABLE_MARKER();\
MOZ_CRASH(MOZ_CRASH_UNREACHABLE); \
} while (0)
MOZ_ASSUME_UNREACHABLE_MARKER tells the compiler feel free to arbitrarily
miscompile this, and anything from that point on in this branch, as you may
assume that this code is unreachable. So it doesn't really serve any
purpose to add a MOZ_CRASH after a MOZ_ASSUME_UNREACHABLE_MARKER.
MOZ_OPTIMIZE_FOR_UNREACHABLE?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: MOZ_ASSUME_UNREACHABLE is being misused
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.
-Jeff
- Original Message -
From: Steve Fink sf...@mozilla.com
To: Benoit Jacob jacob.benoi...@gmail.com
Cc: Chris Peterson cpeter...@mozilla.com, dev-platform
dev-platform@lists.mozilla.org
Sent: Friday, March 28, 2014 1:20:39 PM
Subject: Re: MOZ_ASSUME_UNREACHABLE is being misused
On Fri 28 Mar 2014 01:05:34 PM PDT, Benoit Jacob wrote:
2014-03-28 13:23 GMT-04:00 Chris Peterson cpeter...@mozilla.com:
On 3/28/14, 12:25 PM, Benoit Jacob wrote:
Should we give typical code a macro that does what they want and sounds
like what they want? Really, what typical code wants is a no-operation
instead of undefined-behavior; now, that is exactly the same as
MOZ_ASSERT(false, error). Maybe this syntax is unnecessarily annoying,
and it would be worth adding a macro for that, i.e. similar to MOZ_CRASH
but only affecting DEBUG builds? What would be a good name for it? Is it
worth keeping a close analogy with the unreachable-marker macro to steer
people away from it --- e.g. maybe MOZ_UNREACHABLE_NO_OPERATION or even
just MOZ_UNREACHABLE? So that people couldn't miss it when they look for
UNREACHABLE macros?
How about replacing MOZ_ASSUME_UNREACHABLE with two new macros like:
#define MOZ_ASSERT_UNREACHABLE() \
MOZ_ASSERT(false, MOZ_ASSERT_UNREACHABLE)
#define MOZ_CRASH_UNREACHABLE() \
do { \
MOZ_ASSUME_UNREACHABLE_MARKER();\
MOZ_CRASH(MOZ_CRASH_UNREACHABLE); \
} while (0)
MOZ_ASSUME_UNREACHABLE_MARKER tells the compiler feel free to arbitrarily
miscompile this, and anything from that point on in this branch, as you may
assume that this code is unreachable. So it doesn't really serve any
purpose to add a MOZ_CRASH after a MOZ_ASSUME_UNREACHABLE_MARKER.
MOZ_OPTIMIZE_FOR_UNREACHABLE?
___
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
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. -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: MOZ_ASSUME_UNREACHABLE is being misused
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! 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 3/28/14, 4:05 PM, Benoit Jacob wrote:
#define MOZ_CRASH_UNREACHABLE() \
do { \
MOZ_ASSUME_UNREACHABLE_MARKER();\
MOZ_CRASH(MOZ_CRASH_UNREACHABLE); \
} while (0)
MOZ_ASSUME_UNREACHABLE_MARKER tells the compiler feel free to arbitrarily
miscompile this, and anything from that point on in this branch, as you may
assume that this code is unreachable. So it doesn't really serve any
purpose to add a MOZ_CRASH after a MOZ_ASSUME_UNREACHABLE_MARKER.
I included MOZ_ASSUME_UNREACHABLE_MARKER because that macro is the
compiler-specific optimize me intrinsic, which I believe was the whole
point of the original MOZ_ASSUME_UNREACHABLE.
AFAIU, MOZ_ASSUME_UNREACHABLE_MARKER crashes on all Gecko platforms, but
I included MOZ_CRASH to ensure the behavior was consistent for all
platforms.
chris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: MOZ_ASSUME_UNREACHABLE is being misused
On 14-03-28 05:14 PM, Benoit Jacob wrote: 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! MOZ_UNDEFINED_BEHAVIOUR() ? Undefined behaviour is usually enough to get C/++ programmers' attention. --m. ___ 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:19 GMT-04:00 Mike Habicher mi...@mozilla.com: MOZ_UNDEFINED_BEHAVIOUR() ? Undefined behaviour is usually enough to get C/++ programmers' attention. I thought about that too; then I remembered that it is only at least a year _after_ some time at Mozilla working on Gecko, that I started appreciating how scary Undefined Behavior is. If I remember correctly, before that, I was misunderstanding this concept as just Implementation-defined behavior i.e. not affecting the behavior of other C++ statements, like Undefined Behavior does. Benoit --m. ___ 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! By the way, here is an anecdote. In some very old versions of GCC, when the compiler identified Undefined Behavior, it emitted system commands to try launching some video games that might be present on the system (see: http://feross.org/gcc-ownage/ ). That actually helped more to raise awareness of what Undefined Behavior means, than any serious explanation... So... maybe MOZ_MAYBE_PLAY_STARCRAFT? 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 3/28/14 5:42 PM, Benoit Jacob wrote: So... maybe MOZ_MAYBE_PLAY_STARCRAFT? I'd like to suggest MOZ_IF_REACHED_EXPLODE_COMPUTER. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
