Skip to content

Conversation

fanquake
Copy link
Member

This is not a hardening specific flag, it should be used at all times, regardless of if hardening is enabled or not. Note that this was still the case here, but having this exist in the hardening flags is confusing, and may lead someone to move it inside one of the use_hardening blocks, where it would become unused, with --disable-hardening.

Noticed while reviewing hebasto#32 (comment).

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 18, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK theuni, hebasto, luke-jr, TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@hebasto
Copy link
Member

hebasto commented Oct 18, 2023

This is not a hardening specific flag, it should be used at all times, regardless of if hardening is enabled or not. Note that this was still the case here, but having this exist in the hardening flags is confusing, and may lead someone to move it inside one of the use_hardening blocks, where it would become unused, with --disable-hardening.

I viewed it "confusing" in the same way while working on CMake-based build system.

Concept ACK.

@hebasto
Copy link
Member

hebasto commented Oct 18, 2023

Should we also consider any of other related bugs mentioned in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111843?

This is not a hardening specific flag, it should be used at all times,
regardless of if hardening is enabled or not. Note that this was
still the case here, but having this exist in the hardening flags is
confusing, and may lead someone to move it inside one of the `use_hardening`
blocks, where it would become unused, with `--disable-hardening`.
@fanquake fanquake force-pushed the move_stack_reuse_core_flags branch from 40350b9 to 8cfa22a Compare October 18, 2023 14:20
@fanquake
Copy link
Member Author

fanquake commented Oct 18, 2023

Should we also consider any of other related bugs mentioned in

I've switched to linking to the meta issue, over a specific bug report, but I'm not sure what else you're suggesting to do.

@theuni
Copy link
Member

theuni commented Oct 18, 2023

ACK 8cfa22a. Agree it's confusing as-is and this better matches the intent.

@DrahtBot DrahtBot requested a review from hebasto October 18, 2023 15:05
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 8cfa22a

@DrahtBot
Copy link
Contributor

Guix builds (on x86_64)

File commit 655dc71
(master)
commit 4284cff
(master and this pull)
guix_build.log cebd814ed065d782... 3c834795531c6e99...
guix_build.log.diff 475dcd577b86fb19...

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

utACK 8cfa22a

Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

ACK 8cfa22a

@fanquake fanquake merged commit 5eb82d5 into bitcoin:master Oct 19, 2023
@fanquake fanquake deleted the move_stack_reuse_core_flags branch October 19, 2023 09:09
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 21, 2023
8cfa22a build: move -fstack-reuse=none to CORE_CXXFLAGS (fanquake)

Pull request description:

  This is not a hardening specific flag, it should be used at all times, regardless of if hardening is enabled or not. Note that this was still the case here, but having this exist in the hardening flags is confusing, and may lead someone to move it inside one of the `use_hardening` blocks, where it would become unused, with `--disable-hardening`.

  Noticed while reviewing hebasto#32 (comment).

ACKs for top commit:
  theuni:
    ACK 8cfa22a. Agree it's confusing as-is and this better matches the intent.
  hebasto:
    ACK 8cfa22a
  luke-jr:
    utACK 8cfa22a
  TheCharlatan:
    ACK 8cfa22a

Tree-SHA512: 74c3219301398361d06b1ef2257fc9ec18055b1661f8733ee909adefee61e458d70991c32adf0e0450905a7ffbddc99799f5fdac894f4896cfade19f961818df
@bitcoin bitcoin locked and limited conversation to collaborators Oct 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants