[dsymutil] Add option to copy swiftmodules built from interface #165293
Conversation
|
@llvm/pr-subscribers-debuginfo Author: Roy Shi (royitaqi) ChangesThe default behavior is to not copy such swiftmodules into the dSYM, as perviously set by 96f95c9. This patch adds the option to override the behavior, so that such swiftmodules are copied into the dSYM. This is useful when the dSYM will be used on a machine which has a different Xcode/SDK. Full diff: https://github.com/llvm/llvm-project/pull/165293.diff 5 Files Affected:
diff --git a/llvm/test/tools/dsymutil/cmdline.test b/llvm/test/tools/dsymutil/cmdline.test
index 1574fe35f5254..0b0bce194d575 100644
--- a/llvm/test/tools/dsymutil/cmdline.test
+++ b/llvm/test/tools/dsymutil/cmdline.test
@@ -14,6 +14,7 @@ CHECK: -fat64
CHECK: -flat
CHECK: -gen-reproducer
CHECK: -help
+CHECK: -include-swiftmodules-from-interface
CHECK: -keep-function-for-static
CHECK: -no-object-timestamp
CHECK: -no-odr
diff --git a/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp b/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
index b91c27e6a0f86..ee1e9060657b0 100644
--- a/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
+++ b/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
@@ -794,9 +794,10 @@ bool DwarfLinkerForBinary::linkImpl(
reportWarning("Could not parse binary Swift module: " +
toString(FromInterfaceOrErr.takeError()),
Obj->getObjectFilename());
- // Only skip swiftmodules that could be parsed and are
- // positively identified as textual.
- } else if (*FromInterfaceOrErr) {
+ // Only skip swiftmodules that could be parsed and are positively
+ // identified as textual. Do so only when the option allows.
+ } else if (*FromInterfaceOrErr &&
+ !Options.IncludeSwiftModulesFromInterface) {
if (Options.Verbose)
outs() << "Skipping compiled textual Swift interface: "
<< Obj->getObjectFilename() << "\n";
diff --git a/llvm/tools/dsymutil/LinkUtils.h b/llvm/tools/dsymutil/LinkUtils.h
index ad5515a04333e..c333a3d4afee0 100644
--- a/llvm/tools/dsymutil/LinkUtils.h
+++ b/llvm/tools/dsymutil/LinkUtils.h
@@ -114,6 +114,13 @@ struct LinkOptions {
/// Whether all remarks should be kept or only remarks with valid debug
/// locations.
bool RemarksKeepAll = true;
+
+ /// Whether or not to copy binary swiftmodules built from textual
+ /// .swiftinterface files into the dSYM bundle. These typically come only
+ /// from the SDK (since textual interfaces require library evolution) and
+ /// thus are a waste of space to copy into the bundle. Turn this on if the
+ /// swiftmodules are different from those in the SDK.
+ bool IncludeSwiftModulesFromInterface = false;
/// @}
LinkOptions() = default;
diff --git a/llvm/tools/dsymutil/Options.td b/llvm/tools/dsymutil/Options.td
index ad35e55e33b12..e99bc12fa7fd8 100644
--- a/llvm/tools/dsymutil/Options.td
+++ b/llvm/tools/dsymutil/Options.td
@@ -202,6 +202,14 @@ def remarks_drop_without_debug: Flag<["--", "-"], "remarks-drop-without-debug">,
"all remarks are kept.">,
Group<grp_general>;
+def include_swiftmodules_from_interface: Flag<["--", "-"], "include-swiftmodules-from-interface">,
+ HelpText<"Whether or not to copy binary swiftmodules built from textual "
+ ".swiftinterface files into the dSYM bundle. These typically come only "
+ "from the SDK (since textual interfaces require library evolution) and "
+ "thus are a waste of space to copy into the bundle. Turn this on if the "
+ "swiftmodules are different from those in the SDK.">,
+ Group<grp_general>;
+
def linker: Separate<["--", "-"], "linker">,
MetaVarName<"<DWARF linker type>">,
HelpText<"Specify the desired type of DWARF linker. Defaults to 'classic'">,
diff --git a/llvm/tools/dsymutil/dsymutil.cpp b/llvm/tools/dsymutil/dsymutil.cpp
index 913077eb0b06d..688f6aaf3d0c9 100644
--- a/llvm/tools/dsymutil/dsymutil.cpp
+++ b/llvm/tools/dsymutil/dsymutil.cpp
@@ -391,6 +391,9 @@ static Expected<DsymutilOptions> getOptions(opt::InputArgList &Args) {
Options.LinkOpts.RemarksKeepAll =
!Args.hasArg(OPT_remarks_drop_without_debug);
+ Options.LinkOpts.IncludeSwiftModulesFromInterface =
+ Args.hasArg(OPT_include_swiftmodules_from_interface);
+
if (opt::Arg *BuildVariantSuffix = Args.getLastArg(OPT_build_variant_suffix))
Options.LinkOpts.BuildVariantSuffix = BuildVariantSuffix->getValue();
|
|
EDIT: The latest buildbot seems to pass. This may have been fixed in main.
|
|
@adrian-prantl Could you kindly review this patch when you have the time? It adds an option to skip the logic you added in 96f95c9. The default behavior remains unchanged. |
|
FYI I plan to merge this patch tomorrow if no one else wants to take a look before then. The patch is simple and has tests, so I don't expect it to break anything. cc @adrian-prantl , @JDevlieghere , @clayborg |
Although I see where you're coming from, I feel obligated to point out that doing so wouldn't be in accordance with our code review policy, unless you got someone to LGTM it in the meantime. Plus all the reviewers were at the Developer Meeting so giving folks a bit more time seems appropriate. This is really @adrian-prantl's domain in dsymutil so I'd like him to sign off on it. This looks fine to me. |
|
TL;DR: SG. Let's give it another week so that folks (esp. @adrian-prantl) have the time to review.
Sorry that you had to jump out and stop me. I appreciate it. I take it as a good chance to refresh my memory about the policy. Actually took me sometime to understand the long English sentences. I think the main spirit is to:
FWIW, I was taking @rmaz 's approval as LGTM. I can see why it's not considered as a strong approval (since @adrian-prantl is the domain owner).
Yep make sense to me, esp. about the Developer Meeting. Would love @adrian-prantl 's review for sure (that's why I requested his review in the first place). Happy to wait. |
Thanks for pointing that out, I hadn't noticed and I blame Github's UI for it. It doesn't show up as approved in the list of PRs, in the PR itself it shows up as a grey checkmark and it's also tucked away under "1 more reviewer" in the side bar. I'm a bit puzzled as to why since he's part of the LLVM org. Sorry I missed it. We don't technically require code owners to sign off (though in practice we often do) so you would've been fine to merge with Richard's approval. |
I believe the reason GitHub is presenting rmaz's review in this way is that they are a member of the org triagers team, but not the committers team, so they do not have write access to the repo. If they had commit / write access, then their review would appear in the usual way you were expecting. |
Thanks for the insights, that makes sense. |
|
Thank you both for the observations and insights. I didn't realize it's a gray checkmark until you pointed out. There is always something to learn everyday. :D You guys heave a great day~ |
|
Sorry for the delayed reply @royitaqi. As @JDevlieghere mentioned I'm still catching up with my review queue after the LLVM dev meeting. This patch is solving a real problem and I don't have an issue with bypassing the check per se, however, I also wanted to share with you that we are concurrently working on a different solution to the same problem: The goal is to remove binary Swift modules from dSYMs altogether, because binary Swift modules — being version locked with the compiler — are at odds with the long-term archive promise of dSYM bundles. Instead, we will rely on binary Swift modules recording their binary Swift module dependencies (similar to how they record their clang dependencies, see swiftlang/swift#84412), and having a link from DWARF to every compile unit's own Swift module (see swiftlang/swift#85100). With that in place, we would then retire the linker's |
|
@adrian-prantl: Thank you very much for sharing your plan. I appreciate it. I don't have much Swift experience, but it sounds like a much better path going forward.
Do you happen to have an ETA of when the transition will finish? This will help us decide if we still want to merge and cherry-pick the current patch for now (and later transit to your solution). |
6 participants
Change
The default behavior is to not copy such swiftmodules into the dSYM, as perviously implemented in 96f95c9. This patch adds the option to override the behavior, so that such swiftmodules can be copied into the dSYM.
This is useful when the dSYM will be used on a machine which has a different Xcode/SDK than where the swiftmodules were built. Without this, when LLDB is asked to "p/po" a Swift variable, the underlying Swift compiler code would rebuild the dependent
.swiftmodulefiles of the Swift stdlibs, which takes ~1 minute in some cases.Test
Shell tests:
--
Manually tested by running dsymutil to build a dSYM with and without the option and then checking the size of the
__swift_astsection.Without change:
With change: