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.

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

remove compile option -fno-stack-protector #529

Closed

Conversation

jsteemann
Copy link
Contributor

@jsteemann jsteemann commented Jul 24, 2023

Removes compile option -fno-stack-protector, which is potentially not needed anymore. This can slightly simplify the build process.

ArangoDB PR: arangodb/arangodb#19464
Performance test run: https://jenkins.arangodb.biz/view/Performance/job/perf-simple-branch/96/

Copy link
Contributor

@MBkkt MBkkt left a comment

Choose a reason for hiding this comment

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

I don't think it's good idea.
Just why? Yes, it should be same by default, but for what reason we need to do something implicit instead of explicit

@jsteemann
Copy link
Contributor Author

I don't think it's good idea. Just why? Yes, it should be same by default, but for what reason we need to do something implicit instead of explicit

This PR removes an explicitly set compile option, so we use less custom options and more rely on the defaults.
IMO this has the following advantages:

  • potentialler safer executables (compiled-in stack protection
  • going with the compiler defaults (more tested/used compiler options rather than less tested/used ones)
  • more uniform builds (there are different ways to build ArangoDB, and currently only some set this option - so we currently have different builds depending on how they are built!)
  • small complexity reduction in build infrastructure (Oskar + CMake)

@MBkkt
Copy link
Contributor

MBkkt commented Jul 27, 2023

potentialler safer executables

Well and potentially slower, our perf tests don't really prove that it is not slower.

going with the compiler defaults (more tested/used compiler options rather than less tested/used ones)

Already done, if it's not default it's strange compiler.

more uniform builds

I don't agree, for different compilers, os etc it could be different defaults, so we implicit could have different in terms of options binaries

small complexity reduction in build infrastructure (Oskar + CMake)

You probably meant oskar + circleci.
I think we can fix it another way.
We can just put it in all non maintainer release builds in our cmake instead of oskar/circleci

@jsteemann
Copy link
Contributor Author

Well and potentially slower, our perf tests don't really prove that it is not slower.

Yes, the potential slowdown is of course a disadvantage. That's why I ran the performance tests.

Already done, if it's not default it's strange compiler.

Why is there a performance difference then when removing the option from our build scripts?

I don't agree, for different compilers, os etc it could be different defaults, so we implicit could have different in terms of options binaries

Agree, but that is true for all other options which we don't set explicitly.

You probably meant oskar + circleci. I think we can fix it another way. We can just put it in all non maintainer release builds in our cmake instead of oskar/circleci

I agree this option, if we want to set it explicitly, should be set by our CMake and not by anything outside.

@MBkkt
Copy link
Contributor

MBkkt commented Jul 27, 2023

That's why I ran the performance tests.

I already wrote, and you ignored it.
"our perf tests don't really prove that it is not slower."

Why is there a performance difference then when removing the option from our build scripts?

For strange/new compiler it could be.
Someone could decide safer by default builds is better and change default.

Agree, but that is true for all other options which we don't set explicitly.

In ideal world all options are explicit. But we couldn't do it, so I suggest to do not make more implicit options

@jsteemann
Copy link
Contributor Author

Closing as obsolete.

@jsteemann jsteemann closed this May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants