Bug 227436 - Remove "function declared ‘static’ but never defined" build warnings since r278971.
Summary: Remove "function declared ‘static’ but never defined" build warnings since r2...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joonghun Park
URL:
Keywords: InRadar
: 227450 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-06-27 23:27 PDT by Joonghun Park
Modified: 2021-06-29 16:49 PDT (History)
9 users (show)

See Also:


Attachments
Patch (3.31 KB, patch)
2021-06-27 23:28 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (1.76 KB, patch)
2021-06-29 09:19 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joonghun Park 2021-06-27 23:27:00 PDT
Static functions should be put in cpp file so that it is visible only within the file, and header file should include extern functions
with single implementation, not static functions with different impls for each cpp files usually.
Comment 1 Joonghun Park 2021-06-27 23:28:46 PDT
Created attachment 432366 [details]
Patch
Comment 2 Michael Catanzaro 2021-06-28 07:58:15 PDT
*** Bug 227450 has been marked as a duplicate of this bug. ***
Comment 3 Michael Catanzaro 2021-06-28 08:00:58 PDT
Comment on attachment 432366 [details]
Patch

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

[5/491] Building CXX object Source/JavaScriptCore/CMakeFi...edSources/unified-sources/UnifiedSource-f2e18ffc-27.cpp.o
In file included from ../../Source/JavaScriptCore/runtime/LiteralParser.cpp:39,
                 from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-f2e18ffc-27.cpp:5:
JavaScriptCore/DerivedSources/KeywordLookup.h:87:27: warning: ‘bool JSC::cannotBeIdentPartOrEscapeStart(LChar)’ declared ‘static’ but never defined [-Wunused-function]
   87 | static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(LChar);
      |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
JavaScriptCore/DerivedSources/KeywordLookup.h:88:27: warning: ‘bool JSC::cannotBeIdentPartOrEscapeStart(UChar)’ declared ‘static’ but never defined [-Wunused-function]
   88 | static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(UChar);
      |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

These warnings were introduced in r278971 "[JSC] Optimize JSON.parse with small data by changing Identifier pool mechanism." Problem is KeywordLookup.h does not define these functions and expects the translation unit that it is #included in to do so. Previously that was only Lexer.cpp. Problem is it's now also #included from LiteralParser.cpp, which does not define cannotBeIdentPartOrEscapeStart(LChar) or cannotBeIdentPartOrEscapeStart(UChar).

> Source/JavaScriptCore/parser/Lexer.cpp:788
> -static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(LChar c)
> +ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(LChar c)

Problem is using ALWAYS_INLINE for a function that's not defined in the header doesn't make sense. We need to either:

 (a) Also remove ALWAYS_INLINE, or
 (b) Move the definitions of these functions to KeywordLookup.h (which is generated by KeywordLookupGenerator.py)
Comment 4 Yusuke Suzuki 2021-06-28 10:29:53 PDT
Comment on attachment 432366 [details]
Patch

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

>> Source/JavaScriptCore/parser/Lexer.cpp:788
>> +ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(LChar c)
> 
> Problem is using ALWAYS_INLINE for a function that's not defined in the header doesn't make sense. We need to either:
> 
>  (a) Also remove ALWAYS_INLINE, or
>  (b) Move the definitions of these functions to KeywordLookup.h (which is generated by KeywordLookupGenerator.py)

Please do not remove ALWAYS_INLINE. They are *super* critical performance-sensitive function, and removing that can cause severe perf impact on several benchmarks.
Comment 5 Michael Catanzaro 2021-06-28 10:33:17 PDT
Comment on attachment 432366 [details]
Patch

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

>>> Source/JavaScriptCore/parser/Lexer.cpp:788
>>> +ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(LChar c)
>> 
>> Problem is using ALWAYS_INLINE for a function that's not defined in the header doesn't make sense. We need to either:
>> 
>>  (a) Also remove ALWAYS_INLINE, or
>>  (b) Move the definitions of these functions to KeywordLookup.h (which is generated by KeywordLookupGenerator.py)
> 
> Please do not remove ALWAYS_INLINE. They are *super* critical performance-sensitive function, and removing that can cause severe perf impact on several benchmarks.

Let's do (b) then.
Comment 6 Michael Catanzaro 2021-06-28 10:46:28 PDT
Hi Joonghun, let me know if you want to submit another patch. Otherwise, I can try.
Comment 7 Joonghun Park 2021-06-28 17:13:25 PDT
(In reply to Michael Catanzaro from comment #6)
> Hi Joonghun, let me know if you want to submit another patch. Otherwise, I
> can try.

Hi Michael, I will do the (b) in the next patchset.
Please review that change:)
Comment 8 Joonghun Park 2021-06-29 00:39:07 PDT
(In reply to Joonghun Park from comment #7)
> (In reply to Michael Catanzaro from comment #6)
> > Hi Joonghun, let me know if you want to submit another patch. Otherwise, I
> > can try.
> 
> Hi Michael, I will do the (b) in the next patchset.
> Please review that change:)

Hi, Michael.

While I was trying to move cannotBeIdentPartOrEscapeStart,
I found that the depedent functions and variables seemed not appropriate to move to KeywordLookup.h.

For example, if we move isSingleCharacterIdentPart(UChar c),
then we need to move isIdentPart(CharacterType c) and typesOfLatin1Characters[256] array too, and it seems that typesOfLatin1Characters  should be used only in Lexer.cpp translation unit.

And if we only move the declaration of isIdentStart(CharacterType c),
then it goes to the same place we are currently in.

So I looked around some articles, 
1) https://stackoverflow.com/questions/5057021/why-are-c-inline-functions-in-the-header
2) https://en.cppreference.com/w/cpp/language/inline

and it seems that inline function's definition doesn't have to be placed in header file as long as all the definitions are identical(that's why usually inline function's definition is put in header, to keep it identical for all cpp files including the header file).

So I wonder about 2 options below.

a) If it's ok to keep the inline definitions in cpp files as it is now,
and just remove static keyword(as the current patchset does).


b) Or maybe simply we can keep 'static inline' keywords as it is now,
and just add empty definitions in LiteralParser.cpp as below.

static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(LChar)
{
    return false;
}

static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(UChar)
{
    return false;
}

I confirmed that doing this removed the build warnings,
warning: ‘bool JSC::cannotBeIdentPartOrEscapeStart()’ declared ‘static’ but never defined [-Wunused-function],
but the cons is that it seems those functions has no meaning in LiteralParser.cpp.
Comment 9 Michael Catanzaro 2021-06-29 06:01:13 PDT
(In reply to Joonghun Park from comment #8)
> So I wonder about 2 options below.
> 
> a) If it's ok to keep the inline definitions in cpp files as it is now,
> and just remove static keyword(as the current patchset does).

I don't know. Under your proposal, these functions would not be defined in LiteralParser.cpp at all. I would avoid this.

> b) Or maybe simply we can keep 'static inline' keywords as it is now,
> and just add empty definitions in LiteralParser.cpp as below.
> 
> static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(LChar)
> {
>     return false;
> }
> 
> static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(UChar)
> {
>     return false;
> }

(b) would definitely be OK. I would make it RELEASE_ASSERT_NOT_REACHED() rather than return false. That should be perfectly safe.
Comment 10 Joonghun Park 2021-06-29 07:54:01 PDT
(In reply to Michael Catanzaro from comment #9)
> (In reply to Joonghun Park from comment #8)
> > So I wonder about 2 options below.
> > 
> > a) If it's ok to keep the inline definitions in cpp files as it is now,
> > and just remove static keyword(as the current patchset does).
> 
> I don't know. Under your proposal, these functions would not be defined in
> LiteralParser.cpp at all. I would avoid this.
> 
> > b) Or maybe simply we can keep 'static inline' keywords as it is now,
> > and just add empty definitions in LiteralParser.cpp as below.
> > 
> > static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(LChar)
> > {
> >     return false;
> > }
> > 
> > static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(UChar)
> > {
> >     return false;
> > }
> 
> (b) would definitely be OK. I would make it RELEASE_ASSERT_NOT_REACHED()
> rather than return false. That should be perfectly safe.

Thank you for your review. I will upload a patchset with (b).
Comment 11 Joonghun Park 2021-06-29 09:19:39 PDT
Created attachment 432484 [details]
Patch
Comment 12 Michael Catanzaro 2021-06-29 12:02:17 PDT
Comment on attachment 432484 [details]
Patch

Maybe Yusuke will have a better proposal for how to reorganize the code to avoid this, but it seems like a pretty good solution for now. Thanks Joonghun!
Comment 13 Joonghun Park 2021-06-29 16:13:24 PDT
(In reply to Michael Catanzaro from comment #12)
> Comment on attachment 432484 [details]
> Patch
> 
> Maybe Yusuke will have a better proposal for how to reorganize the code to
> avoid this, but it seems like a pretty good solution for now. Thanks
> Joonghun!

I got it:) Thank you for your help, Michael. I will land this for now.
Comment 14 EWS 2021-06-29 16:48:54 PDT
Committed r279393 (239258@main): <https://commits.webkit.org/239258@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432484 [details].
Comment 15 Radar WebKit Bug Importer 2021-06-29 16:49:18 PDT
<rdar://problem/79939462>