pass -fno-omit-frame-pointer in support of new backtracer #7042

tomerd · 2023-10-27T22:47:02Z

motivation: new backtracer relies on frame pointers. make sure we do not omit them. this can be removed once the backtracer uses DWARF instead of frame pointers

changes:

introduce new hidden "omitFramePointers" build option to explicit omit frame pointers
unless user specifies omitFramePointers/noOmitFramePointers, pass -fno-omit-frame-pointer to the compiler on Linux only
add and adjust tests

rdar://117578677

motivation: new backtracer relies on frame pointers. make sure we do not omit them. this can be removed once the backtracer uses DWARF instead of frame pointers changes: * intriduce new hidden "omitFramePointers" build option to explicit omit frame pointers * unless omitFramePointers is set to true (false by default), pass -fno-omit-frame-pointer to the compiler * adjust tests

tomerd · 2023-10-27T22:49:08Z

cc @al45tair

tomerd · 2023-10-27T23:01:30Z

not sure if we want to do this on all platforms, but started with that

Sources/SPMBuildCore/BuildParameters/BuildParameters.swift

neonichu

LGTM, only concern would be that we are not doing any checking w.r.t. availability of the option, not sure if that gets us into trouble on any platforms.

tomerd · 2023-10-30T18:40:45Z

@swift-ci test

MaxDesiatov

LGTM, but also curious whether this should only be enabled for specific target platforms?

neonichu · 2023-10-30T19:09:46Z

@swift-ci test windows

compnerd

Please add a platform check. -fno-omit-frame-pointer cannot be passed on all targets (e.g. armv7-unknown-windows-msvc IIRC does object).

tomerd · 2023-10-31T17:30:47Z

@swift-ci test

tomerd · 2023-10-31T17:31:32Z

this is now Linux specific unless users forces it

MaxDesiatov · 2023-10-31T17:35:27Z

@swift-ci test windows

tomerd · 2023-10-31T20:29:04Z

@compnerd ptal

Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift

compnerd

I think that @neonichu's idea to vend this as an enum is a great idea. It would avoid having the logic duplicated in multiple sites.

tomerd · 2023-10-31T23:20:52Z

since optional bool already represents three states, we dont really need another enum. that said, moving the platform logic to BuildParameters::Debugging initializer is a good idea

tomerd · 2023-10-31T23:26:54Z

@swift-ci test

tomerd · 2023-10-31T23:27:07Z

@neonichu @compnerd ptal

tomerd · 2023-11-01T00:18:21Z

@swift-ci test windows

neonichu · 2023-11-01T03:36:36Z

Tests/BuildTests/BuildPlanTests.swift

@@ -3511,18 +3538,68 @@ final class BuildPlanTests: XCTestCase {
            XCTAssertMatch(dep, [.anySequence, "-DDEP", .anySequence])

            let cbar = try result.target(for: "cbar").clangTarget().basicArguments(isCXX: false)
-            XCTAssertMatch(cbar, [.anySequence, "-DCCC=2", "-I\(A.appending(components: "Sources", "cbar", "Sources", "headers"))", "-I\(A.appending(components: "Sources", "cbar", "Sources", "cppheaders"))", "-Icfoo", "-L", "cbar", "-Icxxfoo", "-L", "cxxbar", "-g", .end])
+            XCTAssertMatch(cbar, [.anySequence, "-DCCC=2", "-I\(A.appending(components: "Sources", "cbar", "Sources", "headers"))", "-I\(A.appending(components: "Sources", "cbar", "Sources", "cppheaders"))", "-Icfoo", "-L", "cbar", "-Icxxfoo", "-L", "cxxbar", "-g", "-fno-omit-frame-pointer", .end])


Is this correct?

i think so, why not?

for reference this is for .x86_64Linux

neonichu · 2023-11-01T03:37:37Z

Tests/BuildTests/BuildPlanTests.swift

+
+            let exe = try result.target(for: "exe").swiftTarget().compileArguments()
+            XCTAssertMatch(exe, [.anySequence, "-DFOO", "-g", "-Xcc", "-g", "-Xcc", "-fno-omit-frame-pointer", .end])
+        }


Not sure I like that we're making these tests even more unmaintainable by copy-pasting two extra times, but I guess it's fine...

tomerd · 2023-11-01T04:33:35Z

@swift-ci test macOS

compnerd · 2023-11-01T16:47:56Z

Tests/BuildTests/BuildPlanTests.swift

@@ -3059,7 +3083,7 @@ final class BuildPlanTests: XCTestCase {
            "-fblocks", "-fmodules", "-fmodule-name=lib",
            "-I", Pkg.appending(components: "Sources", "lib", "include").pathString,
            "-fmodules-cache-path=\(buildPath.appending(components: "ModuleCache"))",
-            "-g",
+            "-g"


Why remove the trailing ,? It is convenient.

…7042) motivation: new backtracer relies on frame pointers. make sure we do not omit them. this can be removed once the backtracer uses DWARF instead of frame pointers changes: * introduce new hidden "omitFramePointers" build option to explicit omit frame pointers * unless user specifies omitFramePointers/noOmitFramePointers, pass -fno-omit-frame-pointer to the compiler on Linux only * add and adjust tests rdar://117578677

… (#7049) 5.9 cherry pick of #7042

tomerd requested review from neonichu and MaxDesiatov as code owners October 27, 2023 22:47

MaxDesiatov reviewed Oct 28, 2023

View reviewed changes

Sources/SPMBuildCore/BuildParameters/BuildParameters.swift Outdated Show resolved Hide resolved

neonichu approved these changes Oct 30, 2023

View reviewed changes

fixup

b1aa8f2

MaxDesiatov approved these changes Oct 30, 2023

View reviewed changes

compnerd requested changes Oct 31, 2023

View reviewed changes

fixup

0cf8200

MaxDesiatov requested a review from compnerd October 31, 2023 17:35

tomerd enabled auto-merge (squash) October 31, 2023 20:28

neonichu reviewed Oct 31, 2023

View reviewed changes

Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift Outdated Show resolved Hide resolved

compnerd reviewed Oct 31, 2023

View reviewed changes

fixup

a939322

tomerd requested a review from compnerd November 1, 2023 00:18

neonichu reviewed Nov 1, 2023

View reviewed changes

neonichu approved these changes Nov 1, 2023

View reviewed changes

compnerd approved these changes Nov 1, 2023

View reviewed changes

tomerd merged commit 4375ebb into swiftlang:main Nov 1, 2023

tomerd mentioned this pull request Nov 1, 2023

[5.9] pass -fno-omit-frame-pointer in support of new backtracer (#7042) #7049

Merged

tomerd added a commit that referenced this pull request Nov 7, 2023

[5.9] pass -fno-omit-frame-pointer in support of new backtracer (#7042)…

ef57d9f

… (#7049) 5.9 cherry pick of #7042

pass -fno-omit-frame-pointer in support of new backtracer #7042

pass -fno-omit-frame-pointer in support of new backtracer #7042

Uh oh!

Conversation

tomerd commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomerd commented Oct 27, 2023

Uh oh!

tomerd commented Oct 27, 2023

Uh oh!

Uh oh!

neonichu left a comment

Choose a reason for hiding this comment

Uh oh!

tomerd commented Oct 30, 2023

Uh oh!

MaxDesiatov left a comment

Choose a reason for hiding this comment

Uh oh!

neonichu commented Oct 30, 2023

Uh oh!

compnerd left a comment

Choose a reason for hiding this comment

Uh oh!

tomerd commented Oct 31, 2023

Uh oh!

tomerd commented Oct 31, 2023

Uh oh!

MaxDesiatov commented Oct 31, 2023

Uh oh!

tomerd commented Oct 31, 2023

Uh oh!

Uh oh!

compnerd left a comment

Choose a reason for hiding this comment

Uh oh!

tomerd commented Oct 31, 2023

Uh oh!

tomerd commented Oct 31, 2023

Uh oh!

tomerd commented Oct 31, 2023

Uh oh!

tomerd commented Nov 1, 2023

Uh oh!

neonichu Nov 1, 2023

Choose a reason for hiding this comment

Uh oh!

tomerd Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Nov 1, 2023

Choose a reason for hiding this comment

Uh oh!

neonichu Nov 1, 2023

Choose a reason for hiding this comment

Uh oh!

tomerd commented Nov 1, 2023

Uh oh!

compnerd Nov 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomerd commented Oct 27, 2023 •

edited

Loading

tomerd Nov 1, 2023 •

edited

Loading