Bug 133546 - [JavaScriptCore] FTL buildfix for EFL platform
Summary: [JavaScriptCore] FTL buildfix for EFL platform
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: László Langó
URL:
Keywords:
Depends on:
Blocks: 133571
  Show dependency treegraph
 
Reported: 2014-06-05 06:59 PDT by László Langó
Modified: 2014-06-27 00:29 PDT (History)
11 users (show)

See Also:


Attachments
Patch (4.95 KB, patch)
2014-06-05 07:37 PDT, László Langó
no flags Details | Formatted Diff | Diff
Patch (4.94 KB, patch)
2014-06-11 02:11 PDT, László Langó
no flags Details | Formatted Diff | Diff
Patch (4.85 KB, patch)
2014-06-16 09:49 PDT, László Langó
no flags Details | Formatted Diff | Diff
patch for landing (4.78 KB, patch)
2014-06-26 23:37 PDT, László Langó
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description László Langó 2014-06-05 06:59:53 PDT
Fix compiler warnings that breaks the FTL build on EFL
Comment 1 László Langó 2014-06-05 07:37:36 PDT
Created attachment 232547 [details]
Patch
Comment 2 Filip Pizlo 2014-06-10 09:32:42 PDT
Comment on attachment 232547 [details]
Patch

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

> Source/JavaScriptCore/ftl/FTLLocation.cpp:64
> +    return loc;

It would be far better to just say "return Location();"

> Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.cpp:66
> +#pragma GCC diagnostic ignored "-Wmissing-format-attribute"

Why?
Comment 3 László Langó 2014-06-11 02:02:25 PDT
(In reply to comment #2)
> (From update of attachment 232547 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232547&action=review
> 
> > Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.cpp:66
> > +#pragma GCC diagnostic ignored "-Wmissing-format-attribute"
> 
> Why?

Because I don't know any better. This looks like false positive warning to me, but it breaks the build, so we have to ignore it. Do you have any better idea how to handle this warning?
Comment 4 László Langó 2014-06-11 02:11:23 PDT
Created attachment 232856 [details]
Patch
Comment 5 Filip Pizlo 2014-06-11 08:09:55 PDT
Comment on attachment 232856 [details]
Patch

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

> Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.cpp:66
> +#pragma GCC diagnostic ignored "-Wmissing-format-attribute"

I think you should add the format attribute that it is asking for. Have you tried it?  Also, it would be useful to post the compiler error you're getting so that others could suggest a better fix. In general using pragmas to disable warnings is not an OK way of fixing compiler warnings.
Comment 6 László Langó 2014-06-13 05:59:08 PDT
(In reply to comment #5)
> (From update of attachment 232856 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232856&action=review
> 
> > Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.cpp:66
> > +#pragma GCC diagnostic ignored "-Wmissing-format-attribute"
> 
> I think you should add the format attribute that it is asking for. Have you tried it? Also, it would be useful to post the compiler error you're getting so that others could suggest a better fix.

Ok the problem is the following. We have this logger function in WTF:
void WTFLogAlwaysAndCrash(const char* format, ...) __attribute__((__format__(printf, 1, 2)))

And we have this function in LLVMExports.cpp#56 that has a function pointer parameter:
extern "C" WTF_EXPORT_PRIVATE JSC::LLVMAPI* initializeAndGetJSCLLVMAPI(void (*)(const char*, ...));

Then we call the function with a logger function:
initializeAndGetJSCLLVMAPI(WTFLogAlwaysAndCrash);

And we got the folloing waring message:
/mnt/work/Webkit/Webkit/Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.cpp:66:44: error: parameter might be a candidate for a format attribute [-Werror=suggest-attribute=format]
/mnt/work/Webkit/Webkit/Source/JavaScriptCore/llvm/InitializeLLVMPOSIX.cpp:66:44: error: argument of function call might be a candidate for a format attribute [-Werror=suggest-attribute=format]

This is a bit crazy because it points to the call of 'initializeAndGetJSCLLVMAPI' and not to the call of 'WTFLogAlwaysAndCrash'. We call the callback here:
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/llvm/library/LLVMExports.cpp#L70
You can see that is has format argument (and doesn't have any additional args). So it still looks like a false positive warning message for me. But if someone know a better way to resolve this warning, then please let me know.
Comment 7 László Langó 2014-06-16 09:49:32 PDT
Created attachment 233163 [details]
Patch
Comment 8 Darin Adler 2014-06-26 09:41:52 PDT
Comment on attachment 233163 [details]
Patch

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

> Source/JavaScriptCore/ftl/FTLAbstractHeap.cpp:67
> +        if ((static_cast<unsigned>(1) << i) == m_elementSize) {

It’s much better to write this as 1U rather than static_cast<unsigned>(1).

> Source/JavaScriptCore/ftl/FTLStackMaps.cpp:53
> +    out.printf("0x%016llx", static_cast<long long unsigned>(integer));

We normally call the type "unsigned long long" rather than "long long unsigned".
Comment 9 László Langó 2014-06-26 23:37:45 PDT
Created attachment 233968 [details]
patch for landing
Comment 10 WebKit Commit Bot 2014-06-27 00:29:14 PDT
Comment on attachment 233968 [details]
patch for landing

Clearing flags on attachment: 233968

Committed r170525: <http://trac.webkit.org/changeset/170525>
Comment 11 WebKit Commit Bot 2014-06-27 00:29:21 PDT
All reviewed patches have been landed.  Closing bug.