Set clang flags in lldb, propagate linker options #119

amberdixon · 2020-09-09T16:58:48Z

Debugging apps and tests with transitive swift when cacheable_swiftmodules was enabled was not working for us, because:

the LLDB swift-extra-clang-flags setting was not set, and
Frameworks that generated ASTs were not propagating the appropriate linker options to apps/tests that depended on them. link_inputs is a critical piece to propagate forward to the linker; according to the documentation (https://docs.bazel.build/versions/master/skylark/lib/ObjcProvider.html#link_inputs): "Link time artifacts from dependencies that do not fall into any other category such as libraries or archives. This catch-all provides a way to add arbitrary data (e.g. Swift AST files) to the linker. The rule that adds these is also responsible to add the necessary linker flags to 'linkopt'."

Note that when cacheable_swiftmodules is disabled, libraries are built with the serialize-debugging-options flag, so debugging works just fine. When cacheable_swiftmodules is enabled however, we need to give LLDB some additional information and also make sure all the dependent ASTs are linked into the final app/test binary.

The dsymutil command is being used in the tests to view the debugging sections of the generated app/test binary to ensure ALL necessary ASTs are linked to it.

gyfelton · 2020-09-09T17:07:29Z

rules/xcodeproj.bzl

@@ -506,6 +506,10 @@ Double check your rule declaration for naming or add `xcodeproj-ignore-as-target
        target_settings["SWIFT_ACTIVE_COMPILATION_CONDITIONS"] = " ".join(
            ["\"%s\"" % d for d in defines_without_equal_sign],
        )
+        target_settings["BAZEL_LLDB_SWIFT_EXTRA_CLANG_FLAGS"] = " ".join(


this is the key for getting swift to work again right?

There are two issues: (1) LLDB settings need to be right or else LLDB will dump a bunch of errors and (2) the linker command for the application/test needs to include the add_ast_path option. This linker option needs to get propagated down to the application/test target by setting linkopts in the ObjcProvider returned from the framework target for the dependency. If we don't provide the AST option to the linker, then LLDB will not load all the necessary ASTs for swift debugging to work.

thx for this! I should have caught them on the description section.
Looking back, I wonder how can we know these are the steps needed without Sam's input...

Also did you manually ensure that breakpoints work in test projects in this repo? If not that's fine I can do another round of tests

Sure, I can explain how I went about finding these problems. I first added the line log enable lldb types -f /tmp/types.log to my lldbinit, so that I would get very verbose diagnostic lldb logging in a file called /tmp/types.log

I noticed the linker invocations being made by bazel to generate the final binary did not reference all the AST paths for transitive dependencies. I saw that LLDB was (1) sometimes complaining about missing ASTs and (2) in the types.log file , I noticed that the AST for transitive deps were not being loaded (They were being loaded in the working case.) So I just experimented with adding on the add_ast_path option to the linker and that did the trick.

As far as the actual implementation of this change, Sam noticed that framework.bzl was not propagating linker opts for transitive deps, which would explain why these linker options were not present.

After I made that fix, debugging was still not working. The LLDB output was complaining about an error with importing GPBProtocolBuffers.h. I noticed that one of our projects at Square has a clang flag with GPB in the name (this flag affects the way we build source files that depend on google protobufs.) So I used the lldb setting for extra-clang-flags to add all the necessary clang flags.

I will double check the breakpoints in the test project, but note that debugging in the test project only works when sandboxing is disabled (which requires changing local bazelrc.)

In order to test this change, I updated build.sh to check for the presence of AST files in the symbol table for the binary. I think that should be sufficient.

Thanks for the detailed explanation! Learnt that we can do logging for lldb just like we can do it for SourceKIT (so in future get the logs is the priority so we can not shooting in the dark)
As far as the testing go, I think it might be worth to call out in readme that sandbox needs to be disabled to get debugging working? Definitely a separate concern tho

@amberdixon Can point to me where are the changes on build.sh for presence of AST files? 🙏

@amberdixon Can point to me where are the changes on build.sh for presence of AST files? 🙏

saw the updated checks, that's awesome!

amberdixon · 2020-09-09T17:14:20Z

rules/framework.bzl

        "source",
        "define",
        "include",
+        "link_inputs",
+        "linkopt",


Here is the reference for objcprovider: https://docs.bazel.build/versions/master/skylark/lib/ObjcProvider.html

I searched for the string "link" and "final bundle" on this page to see what fields have to get propagated.

amberdixon · 2020-09-09T17:16:09Z

.bazelrc

@@ -6,9 +6,6 @@ build --incompatible_objc_compile_info_migration
 # so circumvent by not using the sandbox
 build --strategy=Stardoc=standalone

-# Debugging does not work in sandbox mode. Uncomment these lines to turn off sandboxing.
-# build --genrule_strategy=standalone


I left these in here by accident, will revert.

gyfelton

Hmmmm this breaks existing tests because SomeFramework is not found, interesting.

gyfelton · 2020-09-09T17:28:09Z

tools/xcodeproj_shims/installers/lldb-settings.sh

+
+if [ -v BAZEL_LLDB_SWIFT_EXTRA_CLANG_FLAGS ]
+then
+cat <<-END > ~/.lldbinit-source-map


I wonder since we are modifying this file ~/.lldbinit-source-map why not modify ~/.lldbinit as well here? Is it because this is our own file while .lldbinit can be shared by multiple projects/other IDE that uses lldb?

I think it's because the developer owns ~/.lldbinit and should be allowed to put what they want in there without us clobbering it. For example, they might put Tulsi LLDB debug settings in there too or any custom debug settings they want xcode to pick up.

If we ask the developer to explicitly include this other file (~/lldbinit-source-map) in their ~/lldb-init, then they maintain control of the contents of lldbinit.

gyfelton · 2020-09-09T19:44:37Z

i assume you have tested against the downstream project that depends on this project? I am referring to the test script we have to ensure the correct debugging experience in various apps/tests?

gyfelton · 2020-09-09T19:54:21Z

I did a test run for Test-Imports-App under tests/ios/unit-test/test-imports-app, got the following error in debug window:
error: the replacement path doesn't exist: "/Users/abc/Development/rules_ios/tests/ios/xcodeproj/../../../bazel-../external"
In the debug window, I can see variable showing up with correct types but auto complete does not work:

Wonder if this is a regression (actually I don't think so because previously debugging is not even supported entirely)

amberdixon · 2020-09-09T21:56:25Z

I did a test run for Test-Imports-App under tests/ios/unit-test/test-imports-app, got the following error in debug window:
error: the replacement path doesn't exist: "/Users/abc/Development/rules_ios/tests/ios/xcodeproj/../../../bazel-../external"
In the debug window, I can see variable showing up with correct types but auto complete does not work:

Wonder if this is a regression (actually I don't think so because previously debugging is not even supported entirely)

I'm seeing this too, I'll dig in. Not sure if it's regression, this wasn't working too well to begin with :( Note that because of issue #108 , you have to disable sandboxing in this repo's bazelrc to get debugging working locally. That alone is not enough however, because I still see this issue.

amberdixon · 2020-09-09T22:54:58Z

Hmmmm this breaks existing tests because SomeFramework is not found, interesting.

I appear to have green builds now. I was probably missing a few things, but they should be resolved.

amberdixon · 2020-09-09T22:56:11Z

I did a test run for Test-Imports-App under tests/ios/unit-test/test-imports-app, got the following error in debug window:
error: the replacement path doesn't exist: "/Users/abc/Development/rules_ios/tests/ios/xcodeproj/../../../bazel-../external"
In the debug window, I can see variable showing up with correct types but auto complete does not work:

Wonder if this is a regression (actually I don't think so because previously debugging is not even supported entirely)

I'm seeing this too, I'll dig in. Not sure if it's regression, this wasn't working too well to begin with :( Note that because of issue #108 , you have to disable sandboxing in this repo's bazelrc to get debugging working locally. That alone is not enough however, because I still see this issue.

I identified the fix to this issue and will push it shortly. LLDB settings should not point to directories that don't exist! will fix.

amberdixon · 2020-09-09T23:25:52Z

i assume you have tested against the downstream project that depends on this project? I am referring to the test script we have to ensure the correct debugging experience in various apps/tests?

Yes, I have tested with one of our internal projects at Square and confirmed that these changes fix the problem we were seeing with debugging swift dependencies. Before my changes were applied, it was not possible to evaluate variables in the debugger.

segiddins

looks good! left a few comments, feel free to merge once they're addressed

segiddins · 2020-09-09T17:17:38Z

tools/xcodeproj_shims/installers/lldb-settings.sh

+cat <<-END > ~/.lldbinit-source-map
+settings set -- target.swift-extra-clang-flags $BAZEL_LLDB_SWIFT_EXTRA_CLANG_FLAGS
+END
+fi


trailing newline

segiddins · 2020-09-10T01:03:07Z

rules/xcodeproj.bzl

@@ -403,7 +403,7 @@ def _xcodeproj_impl(ctx):
        "BAZEL_BUILD_EXEC": "$BAZEL_STUBS_DIR/build-wrapper",
        "BAZEL_OUTPUT_PROCESSOR": "$BAZEL_STUBS_DIR/output-processor.rb",
        "BAZEL_PATH": ctx.attr.bazel_path,
-        "BAZEL_RULES_IOS_OPTIONS": "--@build_bazel_rules_ios//rules:local_debug_options_enabled --features=-swift.cacheable_swiftmodules --features=-swift.use_global_module_cache",


any particular reason these features are removed?

Because of this change, debugging works in local development, even when cacheable swiftmodules are enabled. Therefore, we don't need to explicitly turn off the cacheable swiftmodules feature ... the developer can decide what to do with the feature in their bazelrc if they wish.

segiddins · 2020-09-10T01:06:02Z

tools/xcodeproj_shims/installers/lldb-settings.sh

 settings set target.sdk-path $SDKROOT
 settings set target.swift-framework-search-paths $FRAMEWORK_SEARCH_PATHS
 END
+
+set +u
+if [[ -n $BAZEL_LLDB_SWIFT_EXTRA_CLANG_FLAGS ]]


could do -n ${BAZEL_LLDB_SWIFT_EXTRA_CLANG_FLAGS:-} so you don't need to turn off -u

thank you!! i was having trouble figuring this out.

segiddins · 2020-09-10T01:06:19Z

tools/xcodeproj_shims/installers/lldb-settings.sh

+if [[ -n $BAZEL_LLDB_SWIFT_EXTRA_CLANG_FLAGS ]]
+then
+set -u
+cat <<-END >> ~/.lldbinit-source-map


nit: indent the contents of conditional bodies

segiddins · 2020-09-10T01:07:05Z

tools/xcodeproj_shims/installers/lldb-settings.sh

+set -u
+
+BAZEL_EXTERNAL_DIRNAME="$BAZEL_WORKSPACE_ROOT/bazel-$(basename "$BAZEL_WORKSPACE_ROOT")/external"
+if [ -d $BAZEL_EXTERNAL_DIRNAME ]


quote the variable in case it contains a space?

segiddins · 2020-09-10T01:07:24Z

tools/xcodeproj_shims/installers/lldb-settings.sh

+if [ -d $BAZEL_EXTERNAL_DIRNAME ]
+then
+cat <<-END >> ~/.lldbinit-source-map
+settings append target.source-map ./external/ $BAZEL_EXTERNAL_DIRNAME


does it need to be quoted?

gyfelton

There are some comments from Sam that are definitely worth fixing

gyfelton · 2020-09-10T16:57:16Z

rules/xcodeproj.bzl

@@ -506,6 +506,10 @@ Double check your rule declaration for naming or add `xcodeproj-ignore-as-target
        target_settings["SWIFT_ACTIVE_COMPILATION_CONDITIONS"] = " ".join(
            ["\"%s\"" % d for d in defines_without_equal_sign],
        )
+        target_settings["BAZEL_LLDB_SWIFT_EXTRA_CLANG_FLAGS"] = " ".join(


Thanks for the detailed explanation! Learnt that we can do logging for lldb just like we can do it for SourceKIT (so in future get the logs is the priority so we can not shooting in the dark)
As far as the testing go, I think it might be worth to call out in readme that sandbox needs to be disabled to get debugging working? Definitely a separate concern tho

…d bundling from framework.bzl.

…pilation_mode=dbg in this repo, so we can test binaries contain debug symbols

…so read the presence of the clang flags variable correctly

amberdixon requested review from gyfelton and segiddins September 9, 2020 16:58

gyfelton reviewed Sep 9, 2020

View reviewed changes

amberdixon commented Sep 9, 2020

View reviewed changes

gyfelton requested changes Sep 9, 2020

View reviewed changes

segiddins approved these changes Sep 10, 2020

View reviewed changes

gyfelton approved these changes Sep 10, 2020

View reviewed changes

amberdixon added 7 commits September 13, 2020 20:06

Set extra swift clang flags in lldb. Propagate options for linking an…

ae3c3ab

…d bundling from framework.bzl.

Revert unnecessary changes to bazelrc. Make sure the bazelrc sets com…

5f59b38

…pilation_mode=dbg in this repo, so we can test binaries contain debug symbols

buildifier fixes

e018f67

Move and fix location of app for post build AST check

0335fff

LLDB should not reference directories that do not exist. It should al…

63479e8

…so read the presence of the clang flags variable correctly

Address PR feedback

847aa85

update tests/ios/xcodeproj/TestWithHostApp

9bf6f0b

amberdixon force-pushed the amber/lldb-extra-clang-flags branch from 1d64da3 to 9bf6f0b Compare September 14, 2020 03:09

amberdixon merged commit 2b83b8c into master Sep 14, 2020

acecilia mentioned this pull request Sep 14, 2020

[xcodeproj] Local breakpoints do not work when sandbox is enabled #108

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set clang flags in lldb, propagate linker options #119

Set clang flags in lldb, propagate linker options #119

amberdixon commented Sep 9, 2020 •

edited

Loading

gyfelton Sep 9, 2020

amberdixon Sep 9, 2020 •

edited

Loading

gyfelton Sep 9, 2020 •

edited

Loading

amberdixon Sep 9, 2020

amberdixon Sep 9, 2020

gyfelton Sep 10, 2020

gyfelton Sep 11, 2020

gyfelton Sep 11, 2020

amberdixon Sep 9, 2020 •

edited

Loading

amberdixon Sep 9, 2020

gyfelton left a comment

gyfelton Sep 9, 2020

amberdixon Sep 9, 2020

gyfelton commented Sep 9, 2020

gyfelton commented Sep 9, 2020

amberdixon commented Sep 9, 2020

amberdixon commented Sep 9, 2020

amberdixon commented Sep 9, 2020

amberdixon commented Sep 9, 2020

segiddins left a comment

segiddins Sep 9, 2020

segiddins Sep 10, 2020

amberdixon Sep 11, 2020

segiddins Sep 11, 2020

segiddins Sep 10, 2020

amberdixon Sep 10, 2020

segiddins Sep 10, 2020

segiddins Sep 10, 2020

segiddins Sep 10, 2020

gyfelton left a comment

gyfelton Sep 10, 2020

Set clang flags in lldb, propagate linker options #119

Set clang flags in lldb, propagate linker options #119

Conversation

amberdixon commented Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

amberdixon Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

gyfelton Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amberdixon Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyfelton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyfelton commented Sep 9, 2020

gyfelton commented Sep 9, 2020

amberdixon commented Sep 9, 2020

amberdixon commented Sep 9, 2020

amberdixon commented Sep 9, 2020

amberdixon commented Sep 9, 2020

segiddins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyfelton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amberdixon commented Sep 9, 2020 •

edited

Loading

amberdixon Sep 9, 2020 •

edited

Loading

gyfelton Sep 9, 2020 •

edited

Loading

amberdixon Sep 9, 2020 •

edited

Loading