Skip to content
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.

Sign up for GitHub

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

Don't clobber existing CMAKE_CXX_FLAGS_RELEASE #7742

Merged
merged 1 commit into from
Jan 23, 2025
Merged

Don't clobber existing CMAKE_CXX_FLAGS_RELEASE #7742

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,19 +149,19 @@ endif()
# they can be properly gc'd. -s: strip symbol. -fno-exceptions -fno-rtti:
# disables exceptions and runtime type.
set(CMAKE_CXX_FLAGS_RELEASE
"-ffunction-sections -fdata-sections -fno-exceptions -fno-rtti"
"-ffunction-sections -fdata-sections -fno-exceptions -fno-rtti ${CMAKE_CXX_FLAGS_RELEASE}"
)
if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -s")
endif()

option(OPTIMIZE_SIZE "Build executorch runtime optimizing for binary size" OFF)
if(OPTIMIZE_SIZE)
# -Os: Optimize for size
set(CMAKE_CXX_FLAGS_RELEASE "-Os ${CMAKE_CXX_FLAGS_RELEASE}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's pretty typical to put the overridable CMAKE_CXX_FLAGS_RELEASE last so it can override -Os/-O2 if users want to; the new order will always override whatever CMAKE_CXX_FLAGS_RELEASE says. But if that's the intent (like the PR description says), then this seems to do it.

But since this is an unusual pattern, please update the comments on these two cases to clarify that we're intentionally overriding the -O flag so that someone doesn't come back and "fix" it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

someone doesn't come back and "fix" it.

I would be happy for someone to come back and change this further; these two OPTIMIZE_SIZE changes were to preserve existing behavior.

Before: we clobber all of CMAKE_CXX_FLAGS_RELEASE on line 152 above.
After: we only clobber the -O setting with -Os or -O2.

I think our setup is deeply weird and OPTIMIZE_SIZE shouldn't exist; I am just following general good practice by sending an incremental diff that makes a small amount of forward progress rather than attempting to fix everything all at once.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After: we only clobber the -O setting with -Os or -O2.

Agreed that it's an improvement. But someone a year from now who's only looking at this line might think "why is -Os last? I'll move it back before the ${CMAKE_CXX_FLAGS_RELEASE}". A comment would help avoid that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hesitant to enshrine behavior I think is wrong and simply haven't gotten around to fixing yet.

# -Os: Optimize for size.
set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -Os")
else()
# -O2: Moderate opt.
set(CMAKE_CXX_FLAGS_RELEASE "-O2 ${CMAKE_CXX_FLAGS_RELEASE}")
set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -O2")
endif()

set(CMAKE_CXX_FLAGS_DEBUG "-O0 -g")
Expand Down
Loading