Bug 93931 – [EFL] Remove alloca usage.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kangil Han]

Fixed compile time warning with respect to array and uninitialized variables.

[Kangil Han]

Created attachment 158214 [details]
patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 158214 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158214&action=review

> Source/WebCore/platform/efl/ScrollbarEfl.cpp:174
> -    char buffer[sizeof(Edje_Message_Float_Set) + sizeof(double)];
> -    Edje_Message_Float_Set* message = reinterpret_cast<Edje_Message_Float_Set*>(buffer);
> +    Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*>(alloca(sizeof(Edje_Message_Float_Set) + sizeof(double)));

Whenever you change weird constructs like this, you must check the logs and use svn blame first -- there is a reason for not using alloca() here.

[Kangil Han]

Created attachment 158256 [details]
patch

Under current edje implementation, it seems we wouldn't avoid malloc usage even in cpp source code. :-(

[Grzegorz Czajkowski]

(In reply to comment #3)
> Created an attachment (id=158256) [details]
> patch
> 
> Under current edje implementation, it seems we wouldn't avoid malloc usage even in cpp source code. :-(

Informal r- from my side because of two reason:

1) According to WebKit-Efl style we should avoid memory allocation with malloc and friends. new operator is preferred. 

2) I don't see any benefits of allocating/freeing memory in this case.

You can get more information here https://bugs.webkit.org/show_bug.cgi?id=72017.

[Kangil Han]

(In reply to comment #4)
> Informal r- from my side because of two reason:
> 
> 1) According to WebKit-Efl style we should avoid memory allocation with malloc and friends. new operator is preferred. 
> 
> 2) I don't see any benefits of allocating/freeing memory in this case.
> 
> You can get more information here https://bugs.webkit.org/show_bug.cgi?id=72017.

Would you please suggest a way to remove build warning message under this circumstances?

[Kangil Han]

Created attachment 158269 [details]
patch

How about this?

[Grzegorz Czajkowski]

(In reply to comment #6)
> Created an attachment (id=158269) [details]
> patch
> 
> How about this?

Thanks for the patch and newly introduced solution how to avoid problems with a little problematic Edje_Message_Float_Set struct.

In the origin code 'buffer' is created on stack, so we don't need to take of freeing memory and possibles memory leaks but compiler complains on 'message' pointer after reinterpret_cast that it is not initialized.

Your patch creates 'buffer' on heap and it's deleted when function returns (delete [] buffer).
Additionally you have to twice cast, char * -> void and then void* -> Edje_Message_Float_Set*. 

Is it really worth to resolve this compilation warning?
Kubo what do you think?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 158269 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158269&action=review

> Source/WebCore/ChangeLog:17
> +        * platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:
> +        (WebCore::HarfBuzzShaper::selectionRect): fix 'toX and fromX may be used uninitialized in this function' warning.

Note that this isn't related to the bug title anymore :-)

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #7)
> Is it really worth to resolve this compilation warning?
> Kubo what do you think?

I tried the following snippet here:

    char buffer[sizeof(Edje_Message_Float_Set) + sizeof(double)];
    Edje_Message_Float_Set *message = reinterpret_cast<Edje_Message_Float_Set*>(buffer);

and neither gcc 4.7.1 nor clang 3.1 with -Wall and -Wextra have raised any warning such as the one Kangil mentions in the ChangeLog.

Kangil, what compiler are you using and what flags are being passed to it when building these files?

[Kangil Han]

(In reply to comment #9)
> I tried the following snippet here:
> 
>     char buffer[sizeof(Edje_Message_Float_Set) + sizeof(double)];
>     Edje_Message_Float_Set *message = reinterpret_cast<Edje_Message_Float_Set*>(buffer);
> 
> and neither gcc 4.7.1 nor clang 3.1 with -Wall and -Wextra have raised any warning such as the one Kangil mentions in the ChangeLog.
> 
> Kangil, what compiler are you using and what flags are being passed to it when building these files?

Here it is. :-)
{
gcc (Ubuntu/Linaro 4.6.1-9ubuntu3) 4.6.1

/home/hankangil/dev/WebKit/webkit-gtk/WebKit/Source/WebCore/platform/efl/ScrollbarEfl.cpp:184:23: warning: array subscript is above array bounds [-Warray-bounds]
/home/hankangil/dev/WebKit/webkit-gtk/WebKit/Source/WebCore/platform/efl/ScrollbarEfl.cpp:186:23: warning: array subscript is above array bounds [-Warray-bounds]
}

(In reply to comment #8)
> Note that this isn't related to the bug title anymore :-)

I will select better title after figuring out the direction how I could resolve build warning with respect to strange edje message stuff. ;-)

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #10)
> > and neither gcc 4.7.1 nor clang 3.1 with -Wall and -Wextra have raised any warning such as the one Kangil mentions in the ChangeLog.
> > 
> > Kangil, what compiler are you using and what flags are being passed to it when building these files?
> 
> Here it is. :-)
> {
> gcc (Ubuntu/Linaro 4.6.1-9ubuntu3) 4.6.1

Right. I've tried the following snippet with gcc 4.6.3 and it didn't produce any warnings either:

#include <Edje.h>

int main()
{
    char buffer[sizeof(Edje_Message_Float_Set) + sizeof(double)];
    Edje_Message_Float_Set* message = reinterpret_cast<Edje_Message_Float_Set*>(buffer);

    message->count = 2;
    message->val[0] = 0.0;
    message->val[1] = 1.0;

    return 0;
}

Does this code make your compiler produce any kind of warning?

[Kangil Han]

(In reply to comment #11)
> Does this code make your compiler produce any kind of warning?

I used 'g++ (Ubuntu/Linaro 4.6.1-9ubuntu3) 4.6.1' and found *NO* error on your code snippet. This is very interesting!!

[Kangil Han]

Here is interesting result I've found. It depends on gcc optimization flag.

-O0(no optimization) : no warning message.
-O1 : no warning message.
-O2 : test.cpp:16:19: warning: array subscript is above array bounds [-Warray-bounds]
-O3(actually used in efl build) : test.cpp:16:19: warning: array subscript is above array bounds [-Warray-bounds]

Given circumstances, my two cents on this, I know edje has implemented unusually, using *new* keyword would be unavoidable.

rakuco, grzegorz: how do you think about this?

[Raphael Kubo da Costa (:rakuco)]

OK, I've managed to reproduce that warning with my code snippet by calling edje_object_message_send() in it (clang still doesn't produce any warning).

There are two possible solutions here:

1) Use plain old malloc() here specifying the amount of bytes we want to allocate and avoiding all those casts.

2) Refining Kangil's proposed patch.

Solution 2) is interesting because the static_cast<void*>() is not necessary, so we can remove some bloat. We could also pass the allocated memory to an OwnPtr, thus avoiding the need to free the memory manually.

Kangil, can you produce a new patch following those suggestions (please also refrain from changing variable names to keep it as short as possible)?

[Kangil Han]

(In reply to comment #14)
> OK, I've managed to reproduce that warning with my code snippet by calling edje_object_message_send() in it (clang still doesn't produce any warning).
> 
> There are two possible solutions here:
> 
> 1) Use plain old malloc() here specifying the amount of bytes we want to allocate and avoiding all those casts.
> 
> 2) Refining Kangil's proposed patch.
> 
> Solution 2) is interesting because the static_cast<void*>() is not necessary, so we can remove some bloat. We could also pass the allocated memory to an OwnPtr, thus avoiding the need to free the memory manually.
> 
> Kangil, can you produce a new patch following those suggestions (please also refrain from changing variable names to keep it as short as possible)?

Sure, I will get back with an updated one. ;-)
Thanks!

[Grzegorz Czajkowski]

(In reply to comment #14)
> OK, I've managed to reproduce that warning with my code snippet by calling edje_object_message_send() in it (clang still doesn't produce any warning).
> 
> There are two possible solutions here:
> 
> 1) Use plain old malloc() here specifying the amount of bytes we want to allocate and avoiding all those casts.
> 
How about using new placement instead? Does it produce warning too? I couldn't verify it as I am facing problem to access svn.enlightenment.org and git.webkit.org :/ sorry

char* buffer = new char[sizeof(Edje_Message_Float_Set) + sizeof(double)])
Edje_Message_Float_Set* message = new(buffer) Edje_Message_Float_Set;

This plus smart pointer may be another solution for us.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #16)
> How about using new placement instead? Does it produce warning too?

I don't think you can use placement new here, as Edje_Message_Float_Set is a C type that does not have a constructor or a placement new overload.

[Grzegorz Czajkowski]

(In reply to comment #17)
> (In reply to comment #16)
> > How about using new placement instead? Does it produce warning too?
> 
> I don't think you can use placement new here, as Edje_Message_Float_Set is a C type that does not have a constructor or a placement new overload.

Hmm, I've been trying to use this one at https://bugs.webkit.org/show_bug.cgi?id=72017 (see 5th patch). As I remember it has worked.

[Kangil Han]

(In reply to comment #18)
> Hmm, I've been trying to use this one at https://bugs.webkit.org/show_bug.cgi?id=72017 (see 5th patch). As I remember it has worked.

Don't worry. I will test your code snippet and get back with result. :-)

[Kangil Han]

grzegorz's code snippet works. :-)

rakuco, grzegorz: we can pick better one. which one would you prefer?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #20)
> grzegorz's code snippet works. :-)

Hmm, it was failing here because <new> had to be included in that snippet I was using.

Go for the placement new version, as it is cleaner and conveys the intents better than the casts. Just for safety, please #include <new> as well to make sure the global placement new overload is available.

[Kangil Han]

(In reply to comment #21)
> Hmm, it was failing here because <new> had to be included in that snippet I was using.
> 
> Go for the placement new version, as it is cleaner and conveys the intents better than the casts. Just for safety, please #include <new> as well to make sure the global placement new overload is available.

new version has one drawback.
it seems we couldn't use OwnPtr for this case.
Would you prefer explicit *delete* usage?

[Kangil Han]

Created attachment 159017 [details]
patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 159017 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159017&action=review

> Source/WebCore/ChangeLog:3
> +        [EFL] Remove alloca usage and fix compile warning in WebCore

The original title about removing warnings from WebCore makes more sense here, as removing alloca is part of this process.

> Source/WebCore/ChangeLog:9
> +        To keep portable source code, alloca has been discouraged.
> +        Therefore, this patch removed alloca usage from EFL port.

I would mention here that we're unifying the approaches for the creation of Edje_Message_Float_Set messages, and getting rid of alloca is part of it, since it is not portable. It'd also be good to mention the compiler warnings you were getting.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:360
> +    edje_object_message_send(entry->o, EDJE_MESSAGE_FLOAT_SET, 0, msg);

See the comment in line 327; you do not need to send this message unconditionally.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:450
> +            EINA_LOG_ERR("Could not set file: %s", errmsg);

Not really related to this patch.

> Source/WebCore/platform/efl/ScrollbarEfl.cpp:175
> +    Edje_Message_Float_Set* msg = new(buffer.get()) Edje_Message_Float_Set;

The renaming of this variable makes the patch needlessly bigger.

[Kangil Han]

(In reply to comment #24)
> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:360
> > +    edje_object_message_send(entry->o, EDJE_MESSAGE_FLOAT_SET, 0, msg);
> 
> See the comment in line 327; you do not need to send this message unconditionally.
> 

I couldn't see your comment in line 327. What was that about?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #25)
> I couldn't see your comment in line 327. What was that about?

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/efl/RenderThemeEfl.cpp#L327

[Kangil Han]

Created attachment 159060 [details]
patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 159060 [details]
patch

Looks great, thanks!

[Kangil Han]

(In reply to comment #28)
> (From update of attachment 159060 [details])
> Looks great, thanks!

Thanks for great review, too!

[Grzegorz Czajkowski]

Informal r+ from my side too.

[Kangil Han]

(In reply to comment #30)
> Informal r+ from my side too.

Thanks grzegorz. Your suggestion(using *new*) has helped me a lot. :-)

[Carlos Garcia Campos]

Comment on attachment 159060 [details]
patch

LGTM

[WebKit Review Bot]

Comment on attachment 159060 [details]
patch

Clearing flags on attachment: 159060

Committed r125883: <http://trac.webkit.org/changeset/125883>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.