Bug 185801 – dump-class-layout reports wrong padding in many cases

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

After r229291 the results of dump-class-layout seem suspect.

Before:
 18$ $ ./Tools/Scripts/dump-class-layout -c Release JavaScriptCore SourceProvider
Found 1 types matching "SourceProvider" in "/Volumes/Data/Development/apple/webkit/OpenSource/WebKitBuild/Release/JavaScriptCore.framework/JavaScriptCore" for x86_64
  +0 { 80} SourceProvider
  +0 <  8>     __vtbl_ptr_type * _vptr;
  +8 {  4}     WTF::RefCounted<JSC::SourceProvider>
  +8 {  4}         WTF::RefCountedBase
  +8 <  4>             unsigned int m_refCount;
 +12 <  4>     <PADDING>
 +16 < 16>     JSC::SourceOrigin m_sourceOrigin;
 +16 <  8>         WTF::String m_string;
 +16 <  8>             WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> > m_impl;
 +16 <  8>                 WTF::DumbPtrTraits<WTF::StringImpl>::StorageType m_ptr;
 +24 <  8>         WTF::RefPtr<JSC::ScriptFetcher, WTF::DumbPtrTraits<JSC::ScriptFetcher> > m_fetcher;
 +24 <  8>             WTF::DumbPtrTraits<JSC::ScriptFetcher>::StorageType m_ptr;
 +32 <  8>     WTF::String m_url;
 +32 <  8>         WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> > m_impl;
 +32 <  8>             WTF::DumbPtrTraits<WTF::StringImpl>::StorageType m_ptr;
 +40 <  8>     WTF::String m_sourceURLDirective;
 +40 <  8>         WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> > m_impl;
 +40 <  8>             WTF::DumbPtrTraits<WTF::StringImpl>::StorageType m_ptr;
 +48 <  8>     WTF::String m_sourceMappingURLDirective;
 +48 <  8>         WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> > m_impl;
 +48 <  8>             WTF::DumbPtrTraits<WTF::StringImpl>::StorageType m_ptr;
 +56 <  8>     WTF::TextPosition m_startPosition;
 +56 <  4>         WTF::OrdinalNumber m_line;
 +56 <  4>             int m_zeroBasedValue;
 +60 <  4>         WTF::OrdinalNumber m_column;
 +60 <  4>             int m_zeroBasedValue;
 +64 <  4>     JSC::SourceProviderSourceType m_sourceType;
 +68 <  1>     bool:1 m_validated;
 +69 <  3>     <PADDING>
 +72 <  8>     uintptr_t:63 m_id;
Total byte size: 80
Total pad bytes: 7
Padding percentage: 8.75 %

After:

 16$ $ ./Tools/Scripts/dump-class-layout -c Release JavaScriptCore SourceProvider
Found 1 types matching "SourceProvider" in "/Volumes/Data/Development/apple/webkit/OpenSource/WebKitBuild/Release/JavaScriptCore.framework/JavaScriptCore" for x86_64
  +0 { 80} SourceProvider
  +0 <  8>     __vtbl_ptr_type * _vptr;
  +8 {  4}     WTF::RefCounted<JSC::SourceProvider>
  +8 {  4}         WTF::RefCountedBase
  +8 <  4>             unsigned int m_refCount;
 +12 <  4>     <PADDING>
 +16 < 16>     JSC::SourceOrigin m_sourceOrigin;
 +16 <  8>         WTF::String m_string;
 +16 <  8>             WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> > m_impl;
 +16 <  8>                 WTF::DumbPtrTraits<WTF::StringImpl>::StorageType m_ptr;
 +24 <  8>         WTF::RefPtr<JSC::ScriptFetcher, WTF::DumbPtrTraits<JSC::ScriptFetcher> > m_fetcher;
 +24 <  8>     <PADDING>
 +32 <  8>     WTF::String m_url;
 +32 <  8>     <PADDING>
 +40 <  8>     WTF::String m_sourceURLDirective;
 +40 <  8>     <PADDING>
 +48 <  8>     WTF::String m_sourceMappingURLDirective;
 +48 <  8>     <PADDING>
 +56 <  8>     WTF::TextPosition m_startPosition;
 +56 <  4>         WTF::OrdinalNumber m_line;
 +56 <  4>             int m_zeroBasedValue;
 +60 <  4>         WTF::OrdinalNumber m_column;
 +60 <  4>     <PADDING>
 +64 <  4>     JSC::SourceProviderSourceType m_sourceType;
 +68 <  1>     bool:1 m_validated;
 +69 <  3>     <PADDING>
 +72 <  8>     uintptr_t:63 m_id;
Total byte size: 80
Total pad bytes: 43
Padding percentage: 53.75 %

[Simon Fraser (smfr)]

There are various things wrong here (testing on JSC::PostfixNode)
1. lldb reports the size of zero-sized classes as 1, throwing off padding.
2. The script adds padding to nested classes even when the compiler compacts subsequent fields
3. The 'seenOffset' bailing is wrong.

[Simon Fraser (smfr)]

We need some tests. I seem to recall that Dan made one, but can't find it.

[Daniel Bates]

(In reply to Simon Fraser (smfr) from comment #2)
> We need some tests. I seem to recall that Dan made one, but can't find it.

You’re thinking of bug #183744. It adds the infrastructure that could be used to write tests for dump-class-layout.

[Simon Fraser (smfr)]

(In reply to Simon Fraser (smfr) from comment #1)
> There are various things wrong here (testing on JSC::PostfixNode)
> 1. lldb reports the size of zero-sized classes as 1, throwing off padding.
> 2. The script adds padding to nested classes even when the compiler compacts
> subsequent fields
> 3. The 'seenOffset' bailing is wrong.

4. We fail to dump the vtable in some cases, reporting it as padding.

[JF Bastien]

(In reply to Simon Fraser (smfr) from comment #1)
> There are various things wrong here (testing on JSC::PostfixNode)
> 1. lldb reports the size of zero-sized classes as 1, throwing off padding.

Empty objects have to use at least one byte of storage for their address to be distinct, so lldb is reporting the right information.

There's also the empty base optimization to take into account:

https://en.cppreference.com/w/cpp/language/ebo

I'm not sure there's a way to query for padding bits. Even in C++ there's no trait for it, despite my attempts (there's http://en.cppreference.com/w/cpp/types/has_unique_object_representations but it doesn't do what you want).


> 2. The script adds padding to nested classes even when the compiler compacts
> subsequent fields
> 3. The 'seenOffset' bailing is wrong.

[Simon Fraser (smfr)]

There is "clang -cc1 -fdump-record-layouts"

[Simon Fraser (smfr)]

(In reply to JF Bastien from comment #5)
> (In reply to Simon Fraser (smfr) from comment #1)
> > There are various things wrong here (testing on JSC::PostfixNode)
> > 1. lldb reports the size of zero-sized classes as 1, throwing off padding.
> 
> Empty objects have to use at least one byte of storage for their address to
> be distinct, so lldb is reporting the right information.

Right, but they take no space when inherited from, so we need to take this into account.

> There's also the empty base optimization to take into account:
> 
> https://en.cppreference.com/w/cpp/language/ebo

Good to know.

> I'm not sure there's a way to query for padding bits. Even in C++ there's no
> trait for it, despite my attempts (there's
> http://en.cppreference.com/w/cpp/types/has_unique_object_representations but
> it doesn't do what you want).

[Simon Fraser (smfr)]

I have some unit testing going for dump-class-layout but we need to get bug 183744 fixed first.

[Simon Fraser (smfr)]

First unit test in bug 187141.

[Simon Fraser (smfr)]

Created attachment 344032 [details]
Patch

[Simon Fraser (smfr)]

This patch depends on the patch in bug 187141.

[Simon Fraser (smfr)]

Created attachment 344070 [details]
Patch

[EWS Watchlist]

Attachment 344070 [details] did not pass style-queue:


ERROR: Tools/lldb/lldb_dump_class_layout.py:166:  whitespace before '}'  [pep8/E202] [5]
ERROR: Tools/lldb/lldb_dump_class_layout.py:187:  whitespace before '}'  [pep8/E202] [5]
ERROR: Tools/lldb/lldb_dump_class_layout.py:195:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/lldb/lldb_dump_class_layout.py:206:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/lldb/lldb_dump_class_layout.py:255:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/lldb/lldb_dump_class_layout.py:281:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/lldb/lldbWebKitTester/DumpClassLayoutTesting.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 7 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 344070 [details]
Patch

Attachment 344070 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8408847

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 344075 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Daniel Bates]

Comment on attachment 344070 [details]
Patch

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

> Tools/lldb/lldbWebKitTester/DumpClassLayoutTesting.cpp:83
> +class SimpleVirtualClassWithNonVirtualBase : public BasicClassLayout  {

All these “Virtual Class” names are confusing with regards to C++ virtual inheritance. Speaking of which, can you please add a test for virtual inheritance.

[Daniel Bates]

Comment on attachment 344070 [details]
Patch

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

> Tools/lldb/lldb_dump_class_layout.py:47
> +class ansi_colors:
> +    HEADER = '\033[95m'
> +    OKBLUE = '\033[94m'
> +    OKGREEN = '\033[92m'
> +    WARNING = '\033[93m'
> +    FAIL = '\033[91m'
> +    ENDC = '\033[0m'
> +    BOLD = '\033[1m'
> +    UNDERLINE = '\033[4m'

We only make use of OKBLUE,  WARNING, and ENDC in this patch. I do not see much value to the "OK" prefix in "OKBLUE"? What color is WARNING? Given the name of the class, I expected that its constants would be names of colors. Can we come up with a more descriptive name for ENDC? Or even better remove this constant and take my suggestion below about defining a helper function to colorize a string. The class name ansi_colors does not conform to our PEP8 code style guidelines. For your consideration, I suggest that we keep this class as a namespace for color constants (whose vaues are the ANSI escape codes they represent) and define a new function, say colorize(), that takes a string and a color constant and returns a new string prefixed with the ANSI escape code for the color specified by the color constant and suffixed with the ANSI escape code for reset (\033[0m). We could teach this helper function to fallback to returning the string as-is if the terminal does not support color or some global state is set or not set.

If you choose to keep all the constants as they are named then please add a comment above this class that cites <https://stackoverflow.com/questions/287871/print-in-terminal-with-colors/287944#287944>.

[Simon Fraser (smfr)]

Created attachment 344309 [details]
Patch

[EWS Watchlist]

Attachment 344309 [details] did not pass style-queue:


ERROR: Tools/lldb/lldb_dump_class_layout.py:180:  whitespace before '}'  [pep8/E202] [5]
ERROR: Tools/lldb/lldb_dump_class_layout.py:205:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/lldb/lldb_dump_class_layout.py:229:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/lldb/lldb_dump_class_layout.py:263:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/lldb/lldb_dump_class_layout.py:309:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/lldb/lldb_dump_class_layout.py:335:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/lldb/lldbWebKitTester/DumpClassLayoutTesting.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 7 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 344309 [details]
Patch

Attachment 344309 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8442105

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 344311 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Daniel Bates]

Comment on attachment 344309 [details]
Patch

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

> Tools/lldb/lldbWebKitTester/DumpClassLayoutTesting.cpp:277
> +class VirtualInheritingB: public virtual VirtualBase, public BasicClassLayout {

The interesting case for virtual inheritance is:

class B : public virtual A, public virtual C

Where A has declaration:

class A

And C has declaration:

class C : public A

The expected result is one sub object of class A in the layout of B. Can we please add such a unit test?

> Tools/lldb/lldb_dump_class_layout.py:39
> +class ansi_colors:

You did not address my remarks in comment #17 about this class. Did you disagree? If so, can you please explain your disagreemen?

[Daniel Bates]

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

> Tools/lldb/lldbWebKitTester/DumpClassLayoutTesting.cpp:277
> +class VirtualInheritingB: public virtual VirtualBase, public BasicClassLayout {

The interesting case for virtual inheritance is:

class B : public virtual A, public virtual C

Where A has declaration:

class A

And C has declaration:

class C : public A

The expected result is one sub object of class A in the layout of B. Can we please add such a unit test?

> Tools/lldb/lldb_dump_class_layout.py:39
> +class ansi_colors:

You did not address my remarks in comment #17 about this class. Did you disagree? If so, can you please explain your disagreemen?

[Simon Fraser (smfr)]

(In reply to Daniel Bates from comment #23)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344309&action=review
> 
> > Tools/lldb/lldbWebKitTester/DumpClassLayoutTesting.cpp:277
> > +class VirtualInheritingB: public virtual VirtualBase, public BasicClassLayout {
> 
> The interesting case for virtual inheritance is:
> 
> class B : public virtual A, public virtual C
> 
> Where A has declaration:
> 
> class A
> 
> And C has declaration:
> 
> class C : public A

Actually that doesn't compile:

/DumpClassLayoutTesting.cpp:338:83: error: direct base 'VirtualBaseClass' is inaccessible due to ambiguity:
    class ClassWithImmediateVirtualInheritance -> class ClassWithVirtualBase -> class VirtualBaseClass
    class ClassWithImmediateVirtualInheritance -> class VirtualBaseClass [-Werror,-Winaccessible-base]
class ClassWithImmediateVirtualInheritance : public virtual ClassWithVirtualBase, public virtual VirtualBaseClass {
                                                                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<https://stackoverflow.com/questions/4118412/inaccessible-direct-base-caused-by-multiple-inheritance>

My existing example is the classic diamond-pattern for virtual inheritance:
<https://stackoverflow.com/questions/419943/virtual-inheritance>

I'll fix the colors.

[Simon Fraser (smfr)]

Created attachment 344511 [details]
Patch

[EWS Watchlist]

Attachment 344511 [details] did not pass style-queue:


ERROR: Tools/lldb/lldb_dump_class_layout.py:175:  whitespace before '}'  [pep8/E202] [5]
ERROR: Tools/lldb/lldb_dump_class_layout.py:200:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/lldb/lldb_dump_class_layout.py:224:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/lldb/lldb_dump_class_layout.py:258:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/lldb/lldb_dump_class_layout.py:304:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/lldb/lldb_dump_class_layout.py:330:  whitespace before ':'  [pep8/E203] [5]
ERROR: Tools/lldb/lldbWebKitTester/DumpClassLayoutTesting.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 7 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

*** Bug 187141 has been marked as a duplicate of this bug. ***

[Daniel Bates]

Comment on attachment 344511 [details]
Patch

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

> Tools/lldb/lldb_dump_class_layout.py:39
> +class ansi_colors:

Can we name this class using TitleCase?

> Tools/lldb/lldb_dump_class_layout.py:40
> +    OKBLUE = '\033[94m'

What is OKBLUE? Can this just be BLUE?

[EWS Watchlist]

Comment on attachment 344511 [details]
Patch

Attachment 344511 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8464395

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html

[EWS Watchlist]

Created attachment 344517 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/233614/webkit

[Radar WebKit Bug Importer]

<rdar://problem/41949373>