567530 - kill strict-aliasing warnings for LazilyConstructed in threadsafe builds

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Attachment: v1

On Linux the usage of LazilyConstructed and Conditionally classes adds an aliasing warning noise increasing a noise from the builds. To kill these annoying warnings the attached patch removes the classes and replaces their usage with an explicit condition parameter for AutoGCLock and AutoGCUnlock classes.

Comment 1

[Igor Bukanov]

Attachment: v2

v2 fixes misspelling in parameter names

Comment 2

[Luke Wagner [:luke]]

Did bug 558410 not silence these warnings for you?  What version of GCC and settings are you building with?  I build with zero warnings on gcc 4.4.1 opt builds.  These classes are useful; I would prefer not to remove them.

Comment 3

[Igor Bukanov]

With GCC 4.4.4 on Linux and a debug build of the shell:

/home/igor/m/tm/js/src/jscntxt.h: In function ‘JSContext* js_NextActiveContext(JSRuntime*, JSContext*)’:
/home/igor/m/tm/js/src/jscntxt.h:2505: warning: dereferencing pointer ‘<anonymous>’ does break strict-aliasing rules
/home/igor/m/tm/js/src/jstl.h:288: note: initialized from here
/home/igor/m/tm/js/src/jscntxt.h: In function ‘void js_TriggerAllOperationCallbacks(JSRuntime*, JSBool)’:
/home/igor/m/tm/js/src/jscntxt.h:2505: warning: dereferencing pointer ‘<anonymous>’ does break strict-aliasing rules
/home/igor/m/tm/js/src/jstl.h:288: note: initialized from here
jsdate.cpp

Comment 4

[Igor Bukanov]

I would not not insist on the removal of LazilyConstructed and Conditionally even if I have wasted recently one day finding out what was wrong with my changes. Later it turned out that the aliasing problem that GCC has been warning and that was introduced in unrelated code change were particularly badly intercating with my changes leading to bogus code generation. But I would insist on zero aliasing warnings and AFAICS LazilyConstructed is inherently aliasing unsafe.

Comment 5

[Luke Wagner [:luke]]

(In reply to comment #3)
> With GCC 4.4.4 on Linux and a debug build of the shell:

I'll see what I can do; I do strive for zero warnings.  The general technique is to confuse GCC (AlignedStorage<N> was doing the trick until 4.4.4).

(In reply to comment #4)
> But I would
> insist on zero aliasing warnings and AFAICS LazilyConstructed is inherently
> aliasing unsafe.

LazilyConstructed is using the same trick that js::Vector, std::function and every other generic container that wants to do in-place object construction is using: an array of sizeof(T) chars that is only ever *used* as a type T.  IIUC, it is not a strict-aliasing violation for two incompatibly-typed aliases to a single piece of memory to *exist*.  This warning is a known gcc bug, and even hits std libraries:

  http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39390

Comment 6

[Igor Bukanov]

Attachment: test case

Here is a test case that simulates approximately what lock/unlock is doing. Its essence is:

void test1(bool condition, void *p)
{
    if (condition)
        start(p);
    something(p);
    if (condition)
        stop(p);
}

void test2(bool condition, void *p)
{
    AutoStart2 tmp(condition, p);
    something(p);
}

void test3(bool condition, void *p)
{
    Conditionally<AutoStart> tmp(condition, p);
    something(p);
}

Here test1 uses plain C approach, test2 uses an approach from the patch above and test3 uses the Conditionally. I compile the test using GGC 4.4.4 on 64bit Linux:

g++ -O3 -fno-exceptions -fstrict-aliasing -S -c x.cpp

The resulting assembler output is the following:

test1:
_Z5test1bPv:
	pushl	%ebp
	movl	%esp, %ebp
	pushl	%ebx
	subl	$20, %esp
	movl	12(%ebp), %ebx
	cmpb	$0, 8(%ebp)
	jne	.L14
	movl	%ebx, 8(%ebp)
	addl	$20, %esp
	popl	%ebx
	popl	%ebp
	jmp	_Z9somethingPv
	.p2align 4,,7
	.p2align 3
.L14:
	movl	%ebx, (%esp)
	call	_Z5startPv
	movl	%ebx, (%esp)
	call	_Z9somethingPv
	movl	%ebx, 8(%ebp)
	addl	$20, %esp
	popl	%ebx
	popl	%ebp
	jmp	_Z4stopPv

test2:
	pushl	%ebp
	movl	%esp, %ebp
	pushl	%ebx
	subl	$20, %esp
	movl	12(%ebp), %ebx
	cmpb	$0, 8(%ebp)
	jne	.L10
	movl	%ebx, 8(%ebp)
	addl	$20, %esp
	popl	%ebx
	popl	%ebp
	jmp	_Z9somethingPv
	.p2align 4,,7
	.p2align 3
.L10:
	movl	%ebx, (%esp)
	call	_Z5startPv
	movl	%ebx, (%esp)
	call	_Z9somethingPv
	movl	%ebx, 8(%ebp)
	addl	$20, %esp
	popl	%ebx
	popl	%ebp
	jmp	_Z4stopPv



test3:
	pushl	%ebp
	movl	%esp, %ebp
	pushl	%ebx
	subl	$36, %esp
	movl	12(%ebp), %ebx
	cmpb	$0, 8(%ebp)
	movb	$0, -12(%ebp)
	jne	.L6
.L2:
	movl	%ebx, (%esp)
	call	_Z9somethingPv
	cmpb	$0, -12(%ebp)
	je	.L4
	movl	-20(%ebp), %eax
	movl	%eax, (%esp)
	call	_Z4stopPv
.L4:
	addl	$36, %esp
	popl	%ebx
	popl	%ebp
	ret
	.p2align 4,,7
	.p2align 3
.L6:
	movl	%ebx, -20(%ebp)
	movl	%ebx, (%esp)
	call	_Z5startPv
	movb	$1, -12(%ebp)
	jmp	.L2

Notice how test1 and test2 contains the same assembly corresponding to the compiler transforming the code into:

if (condition) {
    start(p);
    do_something(p);
    stop(p);
} else {
    do_something(p);
}

where the else part is implemented using very efficient direct jump. test3 (the one that uses Conditionally) could not use the optimization.

This tells that aliasing warning is not something that can be ignored and shows that violation of the rules leads to the compiler generating slower code. Perhaps in some ideal world the picture would be more rosy and Conditionally would work well and nice. Unfortunately in the real world we have to deal with compiler inefficiencies with Conditionally having non-zero runtime cost.

Comment 7

[Luke Wagner [:luke]]

(In reply to comment #6)
> This tells that aliasing warning is not something that can be ignored and shows
> that violation of the rules leads to the compiler generating slower code.

"violation of the rules"?  I take it you are referring to "the rules of the C++ language".  Did you read comment 5 and have a logical argument as to why this is a violation?  There may indeed be an argument I am overlooking, but "the compiler generates slower code" is not it.  You are comparing a reusable abstraction (LazilyGenerated) with code duplication; it is regrettable, but not surprising, that the compiler is not doing the exact same thing.

> Perhaps in some ideal world the picture would be more rosy and Conditionally
> would work well and nice. Unfortunately in the real world we have to deal with
> compiler inefficiencies with Conditionally having non-zero runtime cost.

Adding the clause "in the real world" is a bit ironic; you are advocating code duplication in the name of saving a few picoseconds before/after *acquiring a lock*.

And by "code duplication", there is more than just AutoLockGC and AutoUnlockGC:
  
  http://mxr.mozilla.org/mozilla-central/search?string=LazilyConstructed

Again, I am fine with removing a particular use if the warning cannot be avoided -- GCC bugs can't just be ignored -- but let's try that before doing something rash.  LazilyConstructed/Conditionally is already useful and will only become more so as we continue RAIIifying SM.

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> I take it you are referring to "the rules of the C++
> language".

I was wrong, there is no "language violation" indeed, just compiler bugs.

> You are comparing a reusable
> abstraction (LazilyGenerated) with code duplication; it is regrettable, but not
> surprising, that the compiler is not doing the exact same thing.

We have to deal with real compilers and their inefficiencies. If a feature is not well supported by the compiler, then we should try to use a workaround especially when it is known that the compiler contains bugs in that area. Code duplication is not a problem if an alternative is somebody wasting time trying to find a bug in the code just to find out mis-compiled code. Or somebody not spotting a warning about a real problem hidden by aliasing noise.  

> And by "code duplication", there is more than just AutoLockGC and AutoUnlockGC:
> 
>   http://mxr.mozilla.org/mozilla-central/search?string=LazilyConstructed

Sorry I should not rush with the removal of LazilyConstructed. But look at the usage of, for example, js::LazilyConstructed<js::AutoArrayRooter>. js::AutoArrayRooter already adds a C++ pattern that the compiler cannot optimize, but at least its constructor/destructor do not contains any ifs. js::LazilyConstructed adds on top of it conditional checks that the compiler also cannot optimize due to aliasing inefficiencies and makes code harder to follow IMO due to inceased level of abstractions. All that can be avoided if one just uses the already available AutoArrayRooter::changeArray. 

> LazilyConstructed/Conditionally is already useful and will
> only become more so as we continue RAIIifying SM.

If we need these classes to bring RAII to SM, than IMO it would be better to come with macros or templates that would automatically build all the necessary classes including the conditional support around the corresponding C-like entry/exit methods, the classes that compilers can optimize.

Comment 9

[Luke Wagner [:luke]]

(In reply to comment #8)
> We have to deal with real compilers and their inefficiencies.

IIUC, the only inefficiency introduced by LazilyConstructed is that the compiler doesn't do the trick to avoid the second branch.  However, you tested for a synthetic example where do_something is tiny.  If do_something was 10's of lines of code I can't imagine that optimization would be applied.  OTOH, if do_something is short and fast (e.g., does not take a lock) and the path is hot (e.g., not on a rare GC path) then it makes perfect sense not to use LazilyConstructed.  However, I suggest we let profiling tell us when to gross up our code.

> and makes code harder to
> follow IMO due to inceased level of abstractions.

Its not an 'increased level of abstraction'; its a new utility.  As with any new utility, you have to learn what it does.  Auto*Rooters are the same way.

> If we need these classes to bring RAII to SM, than IMO it would be better to
> come with macros or templates that would automatically build all the necessary
> classes including the conditional support around the corresponding C-like
> entry/exit methods, the classes that compilers can optimize.

I can't see how generative programming with macros or templates would be any simpler.

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> IIUC, the only inefficiency introduced by LazilyConstructed is that the
> compiler doesn't do the trick to avoid the second branch.  However, you tested
> for a synthetic example where do_something is tiny.  If do_something was 10's
> of lines of code I can't imagine that optimization would be applied.  OTOH, if
> do_something is short and fast (e.g., does not take a lock) and the path is hot
> (e.g., not on a rare GC path) then it makes perfect sense not to use
> LazilyConstructed.  However, I suggest we let profiling tell us when to gross
> up our code.

For me the inefficiency is another demonstration of bugs in GCC. Together with annoying warnings it tells me that using LazilyConstructed with that compiler could lead to nasty surprises. If the warnings/inneficiencies are not fixable, then the feature should be avoided especially since there are alternatives.

> Its not an 'increased level of abstraction'; its a new utility.  As with any
> new utility, you have to learn what it does.  Auto*Rooters are the same way.

I guess it depends on the definition. I agree that it is a new utility, but it adds a level of indirection which increases abstractions.

> I can't see how generative programming with macros or templates would be any
> simpler.

I have not mean "simpler", I've meant a template class that the compiler would be able optimize to the same code that non-RAII version generates and that would also minimize the amount of boilerplate to write Auto classes. I.e. something like

template<typename T, typename Enter, typename Exit>
class AutoClass {
    const bool condition;
    T t;
    explicit AutoClass(const T& t) : condition(true), t(t) { Enter(t); }
    AutoClass(bool condition, const T& t) : condition(false), t(t) { if (condition) Enter(t); }
    ~AutoClass() { if (condition) Exit(t); }
};

Then to create AutoLockGC one would need to write just:

typedef AutoClass<JS_LOCK_GC, JS_UNLOCK_GC> AutoLockGC;

assuming that JS_LOCK_GC/JS_UNLOCK_GC are replaced by a function.

Comment 11

[Igor Bukanov]

Attachment: v2

The new patch removes just Conditionally and updates comments for LazlyConstructed.

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 447142 [details] [diff] [review]
v2

I do not think this is the right fix for the problem; we should try to silence the warning.  I am happy to help; I just built gcc 4.4.4 and it compiles TM tip without warning on standard opt or debug builds.  Are you seeing these warnings with some patch applied?

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> (From update of attachment 447142 [details] [diff] [review])
> I do not think this is the right fix for the problem; we should try to silence
> the warning.  I am happy to help; I just built gcc 4.4.4 and it compiles TM tip
> without warning on standard opt or debug builds.  Are you seeing these warnings
> with some patch applied?

/home/igor/m/tm/js/src/jscntxt.h: In function ‘JSContext* js_NextActiveContext(JSRuntime*, JSContext*)’:
/home/igor/m/tm/js/src/jscntxt.h:2512: warning: dereferencing pointer ‘<anonymous>’ does break strict-aliasing rules
/home/igor/m/tm/js/src/jstl.h:288: note: initialized from here
/home/igor/m/tm/js/src/jscntxt.h: In function ‘void js_TriggerAllOperationCallbacks(JSRuntime*, JSBool)’:
/home/igor/m/tm/js/src/jscntxt.h:2512: warning: dereferencing pointer ‘<anonymous>’ does break strict-aliasing rules
/home/igor/m/tm/js/src/jstl.h:288: note: initialized from here
jsdate.cpp

This is for threadsafe build of js shell with GCC 4.4.4 that comes with Fedora 13.

Note that warnings are rather sensitive to the usage of the class. For example, I have a patch that triggers such warning in jsgc.cpp after changing unrelated code. 

After playing a bit with the example from the comment 6 I see that the source of confusion for the GCC is reinterpret_cast<T *>(&x)->~T() when x is allocated on the stack. I.e. even if Conditionally is written like in:

template <class T>
class Conditionally {

    unsigned long bytes[(sizeof(T) + sizeof(unsigned long) - 1)
                        / sizeof(unsigned long)];
    bool condition;

  public:

    template <class T1>
    Conditionally(bool condition, const T1& t1) : condition(condition) {
        if (condition)
            new(bytes) T(t1);
    }

    ~Conditionally() {
        if (condition)
            reinterpret_cast<T *>(bytes)->~T();
    }
};

GCC still generates a warning depending on the usage of the class and the compiler doesn't optimize the code.

Comment 14

[Luke Wagner [:luke]]

Attachment: patch

Ah, JS_THREADSAFE.  Tweaking AlignedStorage seems to silence the warning on 4.4.1 and 4.4.4.

Comment 15

[Igor Bukanov]

Attachment: test case 2

(In reply to comment #14)
> Created an attachment (id=447577) [details]
> patch
> 
> Ah, JS_THREADSAFE.  Tweaking AlignedStorage seems to silence the warning on
> 4.4.1 and 4.4.4.

This indeed fixes the warning in the current sources. But it does not help a test case that models the new Conditionally. It still generates the warning - see the attached source which I compile with g++ -O3 -fno-exceptions -fstrict-aliasing -Wall -S -c x.cpp. So I suspect we may see a warning again with different usage pattern of the Conditionally. Could you try to silence the warning in the test case?

Comment 16

[Igor Bukanov]

Attachment: alternative to Conditionally

For consideration the patch provides an AutoClass template that builds RAII wrapper on top of enter/exit methods and switches LockGC to use the new class. The class contains the conditional functionality inbuilt and allows to enter/exit arbitrary number of times. This is useful when the lock should be released temporary and when code contains a path with return inside that section. 

RefillDoubleFreeList is an example of that. There straightforward use of AutoLockGC and AutoUnlockGC would mean that the return path inside the inner while loop would first take lock in AutoUnlock destructor and than release it in AutoLock destructor. Explicit enter/exit calls avoids that yet they ensure that when the method returns the function release the lock.

AFAICS GCC is able to generate the same code for the users of the class that it does for for pre-RAII versions.

Comment 17

[Luke Wagner [:luke]]

(In reply to comment #15)
> This indeed fixes the warning in the current sources. But it does not help a
> test case that models the new Conditionally. It still generates the warning -
> see the attached source which I compile with g++ -O3 -fno-exceptions
> -fstrict-aliasing -Wall -S -c x.cpp.

The attached code does not include any of the warning-silencing changes, even the ones already committed.

> So I suspect we may see a warning again

Perhaps, however there are a growing number of uses without warning.  Ultimately, the STL is doing the same trick and it compiles warning-free, so we should be able to too.

(In reply to comment #16)

> This is useful when the lock should be
> released temporary and when code contains a path with return inside that
> section. 

That's nice.  A few comments:
 - AutoClass seems like an overly generic name, especially since it seems like the name will exclusively be used in typedefs.  Perhaps EnterExitGuard?
 - Having the AutoClass store a const T& seems dangerous since const T & binds to rvals.  E.g., it lets this code compile and fail:

  EnterExitGuard<JSRuntime *, X, Y, Z> guard(JS_GetRuntime(cx));

I would store by value; since the guard isn't aliased, the compiler should be smart enough to avoid duplicates on the stack.

Comment 18

[Luke Wagner [:luke]]

So, to be clear, I think the warning-silencing patch should be committed and, while AutoClass/EnterExitGuard isn't necessary, it seems useful in its own right as it allows even more code to be RAIIified.

Comment 19

[Igor Bukanov]

(In reply to comment #17)
> The attached code does not include any of the warning-silencing changes, even
> the ones already committed.

I wanted to know the root of the cause for the warning as using AutoAligned2 looks rather ugly to my taste.

> Perhaps, however there are a growing number of uses without warning. 
> Ultimately, the STL is doing the same trick and it compiles warning-free, so we
> should be able to too.

This is not true as STL applies the casts to the dynamic storage (correct me if I am wrong here) where GCC does not / cannot using some optimizations as it cannot track all the pointers. For the stack storage GCC tries to do that and the casts confuses it.

(In reply to comment #18)
> So, to be clear, I think the warning-silencing patch should be committed and,
> while AutoClass/EnterExitGuard isn't necessary, it seems useful in its own
> right as it allows even more code to be RAIIified.

Fair enough, but the warning silencer needs comments about LazilyConstructed been suboptimal with GCC together with the reference to this bug. Assume r+ with that fixed.

Comment 20

[Luke Wagner [:luke]]

(In reply to comment #19)
> (In reply to comment #17)
> > The attached code does not include any of the warning-silencing changes, even
> > the ones already committed.
> 
> I wanted to know the root of the cause for the warning as using AutoAligned2
> looks rather ugly to my taste.

Then why did you say "But it does not help a test case that models the new Conditionally." (where it = my patch)?  And of course its ugly; the whole point is to confuse the warning.

> > Ultimately, the STL is doing the same trick and it compiles warning-free, so we
> > should be able to too.
> 
> This is not true as STL applies the casts to the dynamic storage (correct me if
> I am wrong here)

std::function is a classic place where in-place allocation is used.  Also I think std::set; see the GCC bug linked to by comment 5.

> Fair enough, but the warning silencer needs comments about LazilyConstructed
> been suboptimal with GCC together with the reference to this bug.

The only thing I could imagine writing that wasn't FUD would be:

 // If you are using LazilyConstructed on a code path so short, fast and hot
 // that the removal of the branch in the destructor could possibly matter,
 // consider using something else; GCC seems to miss that optimization with
 // this class.

Comment 21

[Igor Bukanov]

(In reply to comment #20)
> Then why did you say "But it does not help a test case that models the new
> Conditionally." (where it = my patch)?  And of course its ugly; the whole point
> is to confuse the warning.

I want to know if it is possible to force GCC to avoid the warning and to generate the same code that a non-RAII version is uses. My theory (which could be wrong) is that as long as suboptimal code is generated the warning may resurrect again even after we try to hide them.

> The only thing I could imagine writing that wasn't FUD would be:
> 
>  // If you are using LazilyConstructed on a code path so short, fast and hot
>  // that the removal of the branch in the destructor could possibly matter,
>  // consider using something else; GCC seems to miss that optimization with
>  // this class.

The issue is not only an extra branch. Consider how in the above examples the GCC also uses more stack space. In fact on 64-bit Linux in the above example the compiler uses only registers in the non-Conditional case while reserves 56 bytes of the stack space with Conditional to store copies of the flag. So the comment could be just:

   // GCC seems to miss some optimizations with LazilyConstructed and may
   // generate extra branches/loads/stores. So use it cautionally on hot
   // paths.

Comment 22

[Igor Bukanov]

(In reply to comment #20)
> std::function is a classic place where in-place allocation is used.  Also I
> think std::set; see the GCC bug linked to by comment 5.

Thanks, I have missed that.

Comment 23

[Luke Wagner [:luke]]

Attachment: with comment

Fair enough.

Comment 24

[Igor Bukanov]

(In reply to comment #23)
> Created an attachment (id=448763) [details]
> with comment

I do not see the comments in the attachment ;)

Comment 25

[Luke Wagner [:luke]]

Attachment: with comment for real

Oops, wrong patch!

Comment 26

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/6b21a1d64072

Comment 27

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/6b21a1d64072