Bug 183744 – Add some tests for lldb_webkit.py

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

Tools/lldb/lldb_webkit.py adds various data formatters to LLDB to pretty-print common WTF and WebCore data structures (e.g. WTF::String) as well as exposes other helpful debugging functions in an LLDB session. The script lldb_webkit.py is fragile and can break when the memory layout of these pretty-printable data structures change among other code changes. People that make changes to these data structures may not be using lldb_webkit.py, be aware of its existence, or may not notice the breakage they cause to lldb_webkit.py. We should add tests for lldb_webkit.py to help identify changes that may break it as well as allow developers extending lldb_webkit.py to write correctness tests.

[Daniel Bates]

Created attachment 336054 [details]
Patch

[Daniel Bates]

Created attachment 336055 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/lldb/lldbWebKitTester/main.cpp:26:  Streams are highly discouraged.  [readability/streams] [3]
Total errors found: 1 in 12 files


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

[Daniel Bates]

Created attachment 336057 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/lldb/lldbWebKitTester/main.cpp:26:  Streams are highly discouraged.  [readability/streams] [3]
Total errors found: 1 in 12 files


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

[Daniel Bates]

I did not mark this patch for review since it add ~15 seconds to building and testing :( On my 2.9 GHz Intel Core i7 MacBook Pro (15-inch, 2017) building lldbWebKitTester takes ~6 seconds and running the LLDB tests take ~9 seconds; => test-webkitpy (without --all) now takes ~16 seconds to run. The reason the LLDB tests take so long is because they run serially. If I do not run them serially then I see error output that has the form:

objc[76794]: +[__MDQuery initialize] may have been in progress in another thread when fork() was called. We cannot safely call it or ignore it in the fork() child process. Crashing instead.

Without the LLDB tests, test-webkitpy (without --all) takes ~7 seconds to run.

I am open to ideas/suggestions on how to decreases the build and test time to make it viable to run these tests on the bots by default.

[Daniel Bates]

Created attachment 340870 [details]
Patch

Updated patch that runs the lldb tests in a new Python process to avoid the +[__MDQuery initialize] issue. This allows us to run the lldb tests in parallel. Although this approach speeds up the tests ~20%, running "test-webkitpy" takes a little more than double the amount of time it takes today: ~12.978666666666667s with the lldb tests vs ~7s without the lldb tests. I am open to ideas/suggestions on how we can further speed the lldb tests up.

[Daniel Bates]

(In reply to Daniel Bates from comment #7)
> [...] Although this approach speeds up the tests ~20%, running
> "test-webkitpy" takes a little more than double the amount of time it takes
*a little less than double

[EWS Watchlist]

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


ERROR: Tools/lldb/lldbWebKitTester/main.cpp:26:  Streams are highly discouraged.  [readability/streams] [3]
Total errors found: 1 in 14 files


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

[Alexey Proskuryakov]

Do you know what makes these lldb tests take so much time? Is it launching lldb many times over?

[Daniel Bates]

(In reply to Alexey Proskuryakov from comment #10)
> Do you know what makes these lldb tests take so much time? Is it launching
> lldb many times over?

Yes, it is launching lldb and the target app many times over.

[Daniel Bates]

Created attachment 340913 [details]
Patch and unit tests

Add missing file stateless_process_runner.py.

[EWS Watchlist]

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


ERROR: Tools/lldb/lldbWebKitTester/main.cpp:26:  Streams are highly discouraged.  [readability/streams] [3]
Total errors found: 1 in 15 files


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

[Alexey Proskuryakov]

Would it be reasonable to run the tests in a single lldb instance?

[Daniel Bates]

(In reply to Alexey Proskuryakov from comment #14)
> Would it be reasonable to run the tests in a single lldb instance?

This is what we were doing when we ran them serially in earlier iterations of this patch (e.g. attachment #336057 [details]).

[Alexey Proskuryakov]

That brings me back to question 10 - what's making the tests so slow? If they are slow regardless of whether they start lldb many times, then it's probably something else.

[Daniel Bates]

Created attachment 341110 [details]
[Patch] Instrument unti tests to dump timing info

Apply this diff on top of the patch (attachment #340913 [details]). Ensure you debug built lldbWebKitTester. (You can do this by running ` make debug -C Tools/lldb/lldbWebKitTester` from the top-level WebKit checkout). Then run test-webkitpy.

On my MacBook Pro (Retina, 15-inch, Early 2013) running macOS High Sierra 10.13.3 (17D47) here are my results:

[[
$ Tools/Scripts/test-webkitpy -p -v
Skipping QueueStatusServer tests; the Google AppEngine Python SDK is not installed.
Suppressing most webkitpy logging while running unit tests.
Skipping tests in the following modules or packages because they are really, really, slow:
    webkitpy.common.checkout.scm.scm_unittest
    (https://bugs.webkit.org/show_bug.cgi?id=31818; use --all to include)

[1/13] webkit.messages_unittest.UnsupportedPrecompilerDirectiveTest.test_error_at_elif passed
[2/13] webkit.messages_unittest.UnsupportedPrecompilerDirectiveTest.test_error_at_else passed
[3/13] webkit.messages_unittest.ParsingTest.test_receiver passed     
[4/13] webkit.messages_unittest.HeaderTest.test_receiver_headers passed
[5/13] webkit.messages_unittest.ReceiverImplementationTest.test_receiver_implementations passed
[5/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_16bit_string
	[serial_test_WTFStringImpl_SummaryProvider_16bit_string] Took 0.197887420654ms to find variable
	[serial_test_WTFStringImpl_SummaryProvider_16bit_string] Took 349.189043045ms to run WTFStringImpl_SummaryProvider
[6/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_16bit_string passed
[6/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_16bit_string
	[serial_test_WTFString_SummaryProvider_16bit_string] Took 0.0581741333008ms to find variable
	[serial_test_WTFString_SummaryProvider_16bit_string] Took 6.78706169128ms to run WTFString_SummaryProvider
[7/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_16bit_string passed
[7/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_null_string
	[serial_test_WTFStringImpl_SummaryProvider_null_string] Took 0.0638961791992ms to find variable
	[serial_test_WTFStringImpl_SummaryProvider_null_string] Took 1.42097473145ms to run WTFStringImpl_SummaryProvider
[8/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_null_string passed
[8/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_empty_string
	[serial_test_WTFString_SummaryProvider_empty_string] Took 0.0629425048828ms to find variable
	[serial_test_WTFString_SummaryProvider_empty_string] Took 1.49893760681ms to run WTFString_SummaryProvider
[9/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_empty_string passed
[9/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_null_string
	[serial_test_WTFString_SummaryProvider_null_string] Took 0.0629425048828ms to find variable
	[serial_test_WTFString_SummaryProvider_null_string] Took 1.52897834778ms to run WTFString_SummaryProvider
[10/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_null_string passed
[10/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_empty_string
	[serial_test_WTFStringImpl_SummaryProvider_empty_string] Took 0.0650882720947ms to find variable
	[serial_test_WTFStringImpl_SummaryProvider_empty_string] Took 1.46508216858ms to run WTFStringImpl_SummaryProvider
[11/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_empty_string passed
[11/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_8bit_string
	[serial_test_WTFString_SummaryProvider_8bit_string] Took 0.0619888305664ms to find variable
	[serial_test_WTFString_SummaryProvider_8bit_string] Took 2.22396850586ms to run WTFString_SummaryProvider
[12/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_8bit_string passed
[12/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_8bit_string
	[serial_test_WTFStringImpl_SummaryProvider_8bit_string] Took 0.0631809234619ms to find variable
	[serial_test_WTFStringImpl_SummaryProvider_8bit_string] Took 1.48797035217ms to run WTFStringImpl_SummaryProvider
[13/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_8bit_string passed
Ran 13 tests in 9.402s

OK
]]

[Daniel Bates]

(In reply to Daniel Bates from comment #17)
> lldb_webkit_unittest.TestSummaryProviders.
> serial_test_WTFStringImpl_SummaryProvider_16bit_string
> 	[serial_test_WTFStringImpl_SummaryProvider_16bit_string] Took
> 0.197887420654ms to find variable
> 	[serial_test_WTFStringImpl_SummaryProvider_16bit_string] Took
> 349.189043045ms to run WTFStringImpl_SummaryProvider

For some reason the first time we call code in Tools/lldb/lldb_webkit.py it takes ~349.189043045ms. That is, 349.189043045ms is not specific to the test serial_test_WTFStringImpl_SummaryProvider_16bit_string.

[Daniel Bates]

Here is the output when parallelizing the lldb unit tests:

[[
$ Tools/Scripts/test-webkitpy -p -v
Skipping QueueStatusServer tests; the Google AppEngine Python SDK is not installed.
Suppressing most webkitpy logging while running unit tests.
Skipping tests in the following modules or packages because they are really, really, slow:
    webkitpy.common.checkout.scm.scm_unittest
    (https://bugs.webkit.org/show_bug.cgi?id=31818; use --all to include)

[1/5] webkit.messages_unittest.UnsupportedPrecompilerDirectiveTest.test_error_at_elif passed
[2/5] webkit.messages_unittest.UnsupportedPrecompilerDirectiveTest.test_error_at_else passed
[3/5] webkit.messages_unittest.ParsingTest.test_receiver passed     
[4/5] webkit.messages_unittest.HeaderTest.test_receiver_headers passed
[5/5] webkit.messages_unittest.ReceiverImplementationTest.test_receiver_implementations passed
[5/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFString_SummaryProvider_null_string (+7)
	[stateless_process_test_WTFStringImpl_SummaryProvider_16bit_string] Took 0.221014022827ms to find variable

	[stateless_process_test_WTFString_SummaryProvider_8bit_string] Took 0.229120254517ms to find variable

	[stateless_process_test_WTFStringImpl_SummaryProvider_empty_string] Took 0.221967697144ms to find variable

	[stateless_process_test_WTFString_SummaryProvider_null_string] Took 0.21505355835ms to find variable

	[stateless_process_test_WTFString_SummaryProvider_16bit_string] Took 0.209808349609ms to find variable


	[stateless_process_test_WTFStringImpl_SummaryProvider_null_string] Took 0.21505355835ms to find variable	[stateless_process_test_WTFStringImpl_SummaryProvider_8bit_string] Took 0.197172164917ms to find variable


	[stateless_process_test_WTFString_SummaryProvider_empty_string] Took 0.226020812988ms to find variable
	[stateless_process_test_WTFStringImpl_SummaryProvider_16bit_string] Took 564.28194046ms to run WTFStringImpl_SummaryProvider
	[stateless_process_test_WTFString_SummaryProvider_8bit_string] Took 566.425085068ms to run WTFString_SummaryProvider
	[stateless_process_test_WTFString_SummaryProvider_null_string] Took 566.066980362ms to run WTFString_SummaryProvider
	[stateless_process_test_WTFStringImpl_SummaryProvider_empty_string] Took 578.790903091ms to run WTFStringImpl_SummaryProvider
	[stateless_process_test_WTFStringImpl_SummaryProvider_8bit_string] Took 570.056915283ms to run WTFStringImpl_SummaryProvider
	[stateless_process_test_WTFString_SummaryProvider_16bit_string] Took 577.664136887ms to run WTFString_SummaryProvider
	[stateless_process_test_WTFStringImpl_SummaryProvider_null_string] Took 578.140974045ms to run WTFStringImpl_SummaryProvider
	[stateless_process_test_WTFString_SummaryProvider_empty_string] Took 577.935218811ms to run WTFString_SummaryProvider
[6/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFStringImpl_SummaryProvider_16bit_string passed
[7/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFString_SummaryProvider_8bit_string passed
[8/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFString_SummaryProvider_null_string passed
[9/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFStringImpl_SummaryProvider_null_string passed
[10/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFStringImpl_SummaryProvider_8bit_string passed
[11/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFString_SummaryProvider_16bit_string passed
[12/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFStringImpl_SummaryProvider_empty_string passed
[13/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFString_SummaryProvider_empty_string passed
Ran 13 tests in 5.345s

OK
]]

[Daniel Bates]

Comment on attachment 340913 [details]
Patch and unit tests

Clearing review flag for further investigation.

[Daniel Bates]

As it turns out the way we support test parallelism in test-webkitpy does not permit using class and module fixtures (i.e. using setUpClass()/setUpModule()). This is because we achieve parallelism by breaking down the structure of test classes into a listing of test methods and then have each worker run exactly one test method at a time. This reorganization of test methods is non-standard and explicitly not supported by the Python 2.7 unittest module: "Shared fixtures are not intended to work with suites with non-standard ordering" [1]. As a result of our non-standard ordering (one test method per TestSuite), we invoke setUpClass()/setUpModule() for each test method. 

[1] <https://docs.python.org/2/library/unittest.html#class-and-module-fixtures>

[Daniel Bates]

Created attachment 341210 [details]
Patch and unit tests that run fast

Cache the LLDB debug session to avoid creating the target and launching the process each time we run a test method. Using this patch, it takes ~2.5 seconds to run just the LLDB tests included in the patch and ~9.5 seconds to run all tests with the default invocatino of "test-webkitpy"

[EWS Watchlist]

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


ERROR: Tools/lldb/lldbWebKitTester/main.cpp:26:  Streams are highly discouraged.  [readability/streams] [3]
Total errors found: 1 in 13 files


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

[Alexey Proskuryakov]

Comment on attachment 341210 [details]
Patch and unit tests that run fast

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

> Tools/Scripts/webkitpy/common/system/systemhost.py:55
> +        return self.filesystem.abspath(self.filesystem.join(xcode_developer_directory, '..', 'SharedFrameworks', 'LLDB.framework', 'Versions', 'A', 'Resources', 'Python'))

Is it documented anywhere that LLDB.framework can be used from this location? https://lldb.llvm.org says that the framework is exposed as a public on on macOS, but I don't see it in /S/L/F. And https://lldb.llvm.org/python-reference.html says that it's in /S/L/PF, which doesn't seem correct either.

> Tools/Scripts/webkitpy/test/main.py:60
> +        _log.info("Skipping lldb_webkit tests; could not find '{}'.".format(lldb_webkit_tester_executable))

This seems fairly dangerous, given that only "make" builds build the tool. So one may run the tests and not realize that they are not testing lldb.

Do you intend to add it to all other build scenarios?

> Tools/lldb/lldbWebKitTester/Configurations/Base.xcconfig:1
> +// Copyright (C) 2015-2018 Apple Inc. All rights reserved.

It may be useful to compare config files to some project that's more actively maintained (perhaps WebCore). I don't see anything that is an obvious problem, but there aren't a lot of xcconfig problems that I can readily notice.

> Tools/lldb/lldbWebKitTester/Configurations/Base.xcconfig:68
> +HEADER_SEARCH_PATHS = ${BUILT_PRODUCTS_DIR}/usr/local/include;

What are we including from here?

> Tools/lldb/lldbWebKitTester/main.cpp:28
> +#include <iostream>
> +#include <wtf/text/StringBuilder.h>
> +#include <wtf/text/WTFString.h>

This is an unusual mix of headers. Do we have to use iostream here?

> Tools/lldb/lldbWebKitTester/main.cpp:33
> +// From Tools/TestWebKitAPI/WTFStringUtilities.h

I'm not sure what useful information this line adds. Is it a FIXME: Find a way to share implementation with TestWebKitAPI?

> Tools/lldb/lldb_webkit_unittest.py:2
> +# -*- coding: utf-8 -*-

I see that we have this line in a few source files, but that's rare, and last it was discussed on webkit-dev, I think that the consensus was that we don't add editor specific markup of any kind.

> Tools/lldb/lldb_webkit_unittest.py:85
> +        cls.sbProcess.Kill()

Is it not a child process that will be killed automatically?

> Tools/lldb/lldb_webkit_unittest.py:86
> +        cls.sbTarget.DeleteAllBreakpoints()

Given that this is only called on exit, not sure what benefit this provides.

[Daniel Bates]

(In reply to Alexey Proskuryakov from comment #24)
> Comment on attachment 341210 [details]
> Patch and unit tests that run fast
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341210&action=review
> 
> > Tools/Scripts/webkitpy/common/system/systemhost.py:55
> > +        return self.filesystem.abspath(self.filesystem.join(xcode_developer_directory, '..', 'SharedFrameworks', 'LLDB.framework', 'Versions', 'A', 'Resources', 'Python'))
> 
> Is it documented anywhere that LLDB.framework can be used from this
> location? https://lldb.llvm.org says that the framework is exposed as a
> public on on macOS, but I don't see it in /S/L/F. And
> https://lldb.llvm.org/python-reference.html says that it's in /S/L/PF, which
> doesn't seem correct either.
> 

Modified the function that contained this code to now query lldb for the path to the directory that contains lldb.py:

[[
    def path_to_lldb_python_directory(self):
        if not self.platform.is_mac():
            return ''
        return self.executive.run_command(['xcrun', 'lldb', '--python-path'], return_stderr=False).rstrip()
]]


> > Tools/Scripts/webkitpy/test/main.py:60
> > +        _log.info("Skipping lldb_webkit tests; could not find '{}'.".format(lldb_webkit_tester_executable))
> 
> This seems fairly dangerous, given that only "make" builds build the tool.
> So one may run the tests and not realize that they are not testing lldb.
> 
> Do you intend to add it to all other build scenarios?
> 

Will add logic to check if lldbWebKitTester was built for Release or Debug. If neither exists then will build lldbWebKitTester for Debug.

> > Tools/lldb/lldbWebKitTester/Configurations/Base.xcconfig:1
> > +// Copyright (C) 2015-2018 Apple Inc. All rights reserved.
> 
> It may be useful to compare config files to some project that's more
> actively maintained (perhaps WebCore). I don't see anything that is an
> obvious problem, but there aren't a lot of xcconfig problems that I can
> readily notice.
> 

Will update the xcconfig files in this patch  using WebCore's xccconfig files as a template.

> > Tools/lldb/lldbWebKitTester/Configurations/Base.xcconfig:68
> > +HEADER_SEARCH_PATHS = ${BUILT_PRODUCTS_DIR}/usr/local/include;
> 
> What are we including from here?
> 

Nothing. Will remove.

> > Tools/lldb/lldbWebKitTester/main.cpp:28
> > +#include <iostream>
> > +#include <wtf/text/StringBuilder.h>
> > +#include <wtf/text/WTFString.h>
> 
> This is an unusual mix of headers. Do we have to use iostream here?
> 

No, we do not need to use iostream. Will switch to using stdio.h.

> > Tools/lldb/lldbWebKitTester/main.cpp:33
> > +// From Tools/TestWebKitAPI/WTFStringUtilities.h
> 
> I'm not sure what useful information this line adds. Is it a FIXME: Find a
> way to share implementation with TestWebKitAPI?
> 

Will remove this comment.

> > Tools/lldb/lldb_webkit_unittest.py:2
> > +# -*- coding: utf-8 -*-
> 
> I see that we have this line in a few source files, but that's rare, and
> last it was discussed on webkit-dev, I think that the consensus was that we
> don't add editor specific markup of any kind.
> 

This line is required to tell Python that the source is UTF-8 encoded so that string literals with UTF-8 encoded characters are interpreted correctly. Otherwise, Python interprets source files as ASCII. See <https://www.python.org/dev/peps/pep-0263/> for more details. 

> > Tools/lldb/lldb_webkit_unittest.py:85
> > +        cls.sbProcess.Kill()
> 
> Is it not a child process that will be killed automatically?
> 

I am going to leave this as-is.

It does not seem like good programming practice to depend on program termination/the OS to clean up resources, including child processes, when we can clean them up earlier in the program lifecycle. The lldb_webkit tests are not the only serial tests that we have. I do not see the need to keep the child process around longer than necessary.

On another note, I hope that one day we can reconcile how test-webkitpy runs unit tests such that we can use setUpClass(). When we reconcile this issue then the code in LLDBDebugSession.setup() and LLDBDebugSession.tearDown() can be moved as-is to setUpClass() and tearDownClass(), respectively.

> > Tools/lldb/lldb_webkit_unittest.py:86
> > +        cls.sbTarget.DeleteAllBreakpoints()
> 
> Given that this is only called on exit, not sure what benefit this provides.

Will remove.

[Daniel Bates]

Created attachment 341388 [details]
Patch for landing

[Daniel Bates]

Comment on attachment 341210 [details]
Patch and unit tests that run fast

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

> Tools/lldb/lldb_webkit_unittest.py:65
> +        cls.sbTarget = cls.sbDebugger.CreateTargetWithFileAndArch(str(cls.lldbWebKitTesterExecutable), lldb.LLDB_ARCH_DEFAULT)

Hit a bug with CreateTargetWithFileAndArch(). Switched to CreateTarget(). See <rdar://problem/39960149> for more details.

[Daniel Bates]

Created attachment 341691 [details]
For EWS

[Daniel Bates]

Created attachment 341712 [details]
For EWS

[Daniel Bates]

Created attachment 341714 [details]
For EWS

[Daniel Bates]

Created attachment 341716 [details]
For EWS

[Daniel Bates]

Created attachment 341731 [details]
For EWS

[Daniel Bates]

Created attachment 341733 [details]
For EWS

[Daniel Bates]

Created attachment 341791 [details]
For EWS

[Daniel Bates]

Created attachment 341794 [details]
For EWS

[Daniel Bates]

Committed r232421: <https://trac.webkit.org/changeset/232421>

[Radar WebKit Bug Importer]

<rdar://problem/40737972>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 186240

[Simon Fraser (smfr)]

I had to add -framework Security to OTHER_LDFLAGS to get lldbWebKitTester to link.

[Daniel Bates]

(In reply to Simon Fraser (smfr) from comment #39)
> I had to add -framework Security to OTHER_LDFLAGS to get lldbWebKitTester to
> link.

Are you able to share what linker errors or insight led you to add this dependency?

[Daniel Bates]

(In reply to Daniel Bates from comment #40)
> (In reply to Simon Fraser (smfr) from comment #39)
> > I had to add -framework Security to OTHER_LDFLAGS to get lldbWebKitTester to
> > link.
> 
> Are you able to share what linker errors or insight led you to add this
> dependency?

Never mind.

$ Tools/Scripts/build-lldbwebkittester --debug
...
Ld /Volumes/Data/WebKitDev/OpenSource/WebKitBuild/Debug/lldbWebKitTester normal x86_64
...
Undefined symbols for architecture x86_64:
  "_SecTaskCopyValueForEntitlement", referenced from:
      WTF::processHasEntitlement(char const*) in libWTF.a(Entitlements.o)
  "_SecTaskCreateFromSelf", referenced from:
      WTF::processHasEntitlement(char const*) in libWTF.a(Entitlements.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[Daniel Bates]

Comment on attachment 341794 [details]
For EWS

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

> Tools/Scripts/webkitpy/test/main.py:67
> +    return _host.executive.run_command([build_lldbwebkittester, config.flag_for_configuration(config.default_configuration())], return_exit_code=True) == 0

I am going to change this line to emit the full output on failure:

try:
    _host.executive.run_and_throw_if_fail([build_lldbwebkittester, config.flag_for_configuration(config.default_configuration())], quiet=True)
except ScriptError as e:
    _log.error(e.message_with_output(output_limit=None))
    return False
return True

This will help diagnose the cause of the lldbWebKitTester compile-time error that led to this patch being rolled out in bug #186240. I will re-land this patch with the above change and add -framework Security to OTHER_LDFLAGS[sdk=macosx*] in file Tools/lldb/lldbWebKitTester/Configurations/lldbWebKitTester.xcconfig. I will watch the commit-queue for failure.

[Daniel Bates]

Created attachment 343791 [details]
Patch for landing

[Daniel Bates]

Committed r233299: <https://trac.webkit.org/changeset/233299>

[Daniel Bates]

On Mac Developer Tools Access is required in order for the lldb tests to be able to debug the lldbWebKitTester process. Running the lldb tests will prompt for administrator credentials to enable such access. You can change you system-wide security authorization policy to avoid such prompts by running `/usr/sbin/DevToolsSecurity -enable`. See DevToolsSecurity(1) for more details.

[Simon Fraser (smfr)]

(In reply to Daniel Bates from comment #42)
> Comment on attachment 341794 [details]
> For EWS
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341794&action=review
> 
> > Tools/Scripts/webkitpy/test/main.py:67
> > +    return _host.executive.run_command([build_lldbwebkittester, config.flag_for_configuration(config.default_configuration())], return_exit_code=True) == 0
> 
> I am going to change this line to emit the full output on failure:
> 
> try:
>     _host.executive.run_and_throw_if_fail([build_lldbwebkittester,
> config.flag_for_configuration(config.default_configuration())], quiet=True)
> except ScriptError as e:
>     _log.error(e.message_with_output(output_limit=None))
>     return False
> return True
> 
> This will help diagnose the cause of the lldbWebKitTester compile-time error
> that led to this patch being rolled out in bug #186240. I will re-land this
> patch with the above change and add -framework Security to
> OTHER_LDFLAGS[sdk=macosx*] in file
> Tools/lldb/lldbWebKitTester/Configurations/lldbWebKitTester.xcconfig. I will
> watch the commit-queue for failure.

Nice. I discovered it by building lldbWebKitTester.xcconfig by hand in Xcode.

[Chris Dumez]

This may have broken iOS build:
Ld /Volumes/Data/WebKit/OpenSource/WebKitBuild/Release-iphoneos/lldbWebKitTester normal arm64
    cd /Volumes/Data/WebKit/OpenSource/Tools/lldb/lldbWebKitTester
    /Applications/Xcode.app/Contents/Developer/Toolchains/iOS12.0.xctoolchain/usr/bin/clang++ -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS12.0.Internal.sdk -L/Volumes/Data/WebKit/OpenSource/WebKitBuild/Release-iphoneos -F/Volumes/Data/WebKit/OpenSource/WebKitBuild/Release-iphoneos -iframework /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS12.0.Internal.sdk/System/Library/PrivateFrameworks -filelist /Volumes/Data/WebKit/OpenSource/WebKitBuild/lldbWebKitTester.build/Release-iphoneos/lldbWebKitTester.build/Objects-normal/arm64/lldbWebKitTester.LinkFileList -miphoneos-version-min=12.0 -Xlinker -object_path_lto -Xlinker /Volumes/Data/WebKit/OpenSource/WebKitBuild/lldbWebKitTester.build/Release-iphoneos/lldbWebKitTester.build/Objects-normal/arm64/lldbWebKitTester_lto.o -Xlinker -no_deduplicate -stdlib=libc++ -licucore -lWTF -Xlinker -dependency_info -Xlinker /Volumes/Data/WebKit/OpenSource/WebKitBuild/lldbWebKitTester.build/Release-iphoneos/lldbWebKitTester.build/Objects-normal/arm64/lldbWebKitTester_dependency_info.dat -o /Volumes/Data/WebKit/OpenSource/WebKitBuild/Release-iphoneos/lldbWebKitTester
Undefined symbols for architecture arm64:
  "_CFAllocatorCreate", referenced from:
      WTF::StringImpl::createCFString() in libWTF.a(StringImplCF.o)
  "_CFArrayGetCount", referenced from:
      WTF::currentSearchLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
      WTF::currentTextBreakLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
  "_CFArrayGetValueAtIndex", referenced from:
      WTF::currentSearchLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
      WTF::currentTextBreakLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
  "_CFGetTypeID", referenced from:
      WTF::currentTextBreakLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
  "_CFLocaleCopyPreferredLanguages", referenced from:
      WTF::currentSearchLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
      WTF::currentTextBreakLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
  "_CFLocaleCreateCanonicalLanguageIdentifierFromString", referenced from:
      WTF::currentTextBreakLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
  "_CFPreferencesCopyValue", referenced from:
      WTF::currentTextBreakLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
  "_CFRelease", referenced from:
      WTF::createWithFormatAndArguments(char const*, char*) in libWTF.a(WTFString.o)
      vprintf_stderr_common(char const*, char*) in libWTF.a(Assertions.o)
      WTF::timerFired(__CFRunLoopTimer*, void*) in libWTF.a(MainThreadMac.o)
      WTF::currentSearchLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
      WTF::currentTextBreakLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
  "_CFRetain", referenced from:


At least I cannot link anymore.

[Dawei Fenton (:realdawei)]

This is causing timeouts on Sierra

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/10215/steps/webkitpy-test/logs/stdio

[Daniel Bates]

(In reply to Chris Dumez from comment #47)
> This may have broken iOS build:
> Ld
> /Volumes/Data/WebKit/OpenSource/WebKitBuild/Release-iphoneos/
> lldbWebKitTester normal arm64
>     cd /Volumes/Data/WebKit/OpenSource/Tools/lldb/lldbWebKitTester
>    
> /Applications/Xcode.app/Contents/Developer/Toolchains/iOS12.0.xctoolchain/
> [...]
> libWTF.a(TextBreakIteratorInternalICUMac.o)
>   "_CFRetain", referenced from:
> 
> 
> At least I cannot link anymore.

We should only build lldbWebKitTester for Mac. Committed fix in <https://trac.webkit.org/changeset/233313>.

[Daniel Bates]

(In reply to David Fenton (:realdawei) from comment #48)
> This is causing timeouts on Sierra
> 
> https://build.webkit.org/builders/
> Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/10215/steps/webkitpy-
> test/logs/stdio

Needed to run `sudo /usr/sbin/DevToolsSecurity -enable` on the bot{164, 199}, which services this queue, to allow debugging. See comment 45 for more details.

[Daniel Bates]

(In reply to Daniel Bates from comment #50)
> (In reply to David Fenton (:realdawei) from comment #48)
> > This is causing timeouts on Sierra
> > 
> > https://build.webkit.org/builders/
> > Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/10215/steps/webkitpy-
> > test/logs/stdio
> 
> Needed to run `sudo /usr/sbin/DevToolsSecurity -enable` on the bot{164,
> 199}, which services this queue, to allow debugging. See comment 45 for more
> details.

We will need to enable Developer Tool Access on all Mac and iOS bots that run test-webkitpy.

[Daniel Bates]

(In reply to Daniel Bates from comment #51)
> (In reply to Daniel Bates from comment #50)
> > (In reply to David Fenton (:realdawei) from comment #48)
> > > This is causing timeouts on Sierra
> > > 
> > > https://build.webkit.org/builders/
> > > Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/10215/steps/webkitpy-
> > > test/logs/stdio
> > 
> > Needed to run `sudo /usr/sbin/DevToolsSecurity -enable` on the bot{164,
> > 199}, which services this queue, to allow debugging. See comment 45 for more
> > details.
> 
> We will need to enable Developer Tool Access on all Mac and iOS bots that
> run test-webkitpy.

err, I meant to write:

We will need to enable Developer Tool Access on all Mac test bots that run test-webkitpy.