Skip to content

Fixup omitting frame pointers on various compilers and architectures #7626

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 22, 2017
Merged

Fixup omitting frame pointers on various compilers and architectures #7626

merged 3 commits into from
Feb 22, 2017

Conversation

hughbe
Copy link
Contributor

@hughbe hughbe commented Feb 20, 2017

Commit 1:

  • Add momit-leaf-frame-pointer on i686 architectures
  • There is a comment "Add -momit-leaf-frame-pointer on x86.", so we should do this on x86

Commit 2:

Commit 3:

  • There is a comment "Add -momit-leaf-frame-pointer on x86.", so we should do this on x86_64

@hughbe
Copy link
Contributor Author

hughbe commented Feb 20, 2017

@compnerd PTAL. The second commit I know is correct.
However, the 1st and 3rd commits rely on the comment that we omit frame pointers on x86. It currently seems like this is not the case.
So, either this is the correct fix, or we need to update the comment to say "on x86 and x86_64"

This is x86. See the root CMakeLists.txt file
```
elseif("${CMAKE_SYSTEM_PROCESSOR}" STREQUAL "x86")
set(SWIFT_HOST_VARIANT_ARCH_default "i686")
```
@hughbe
Copy link
Contributor Author

hughbe commented Feb 20, 2017

@swift-ci please clean smoke test

@compnerd
Copy link
Member

The change is correct, but please collapse the patches into a single one before pushing this.

@hughbe hughbe merged commit 781820b into swiftlang:master Feb 22, 2017
@hughbe
Copy link
Contributor Author

hughbe commented Feb 22, 2017

Squash and merge it is. Was unsure what the right thing to do was, so split the commits up :)

@hughbe hughbe deleted the omit-frame-pointers branch February 22, 2017 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants