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

Fix --disable-asm for newer assembly checks/code #13788

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@luke-jr
Copy link
Member

luke-jr commented Jul 28, 2018

Fixes #13759

Also inverts the help (so it shows --disable-asm like other enabled-by-default options, and initialises the flag variables.

@laanwj laanwj added this to the 0.17.0 milestone Jul 30, 2018

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jul 30, 2018

makes sense, utACK 4207c1b

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Aug 8, 2018

utACK 4207c1b

@ken2812221

This comment has been minimized.

Copy link
Member

ken2812221 commented Aug 8, 2018

ACK 4207c1b

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Aug 8, 2018

The variables enable_hwcrc, enable_sse41, enable_avx2, and enable_shani do not control inclusion of assembly code, but C++ with intrinsics. See my comment here: #13759 (comment).

Overall my preference would be to remove that compilation flag, as I don't know under what conditions I'd suggest someone disable it.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Aug 8, 2018

do not control inclusion of assembly code, but C++ with intrinsic

I think many people interpret asm as "special instructions", independent of whether these are emitted though inline assembly or intrinsics.

as I don't know under what conditions I'd suggest someone disable it.

Also not sure on this. Clearly they should be disabled if the compiler doesn't know the intrinsics, but that is already done automatically.

@laanwj laanwj removed this from the 0.17.0 milestone Aug 15, 2018

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Aug 15, 2018

Removing 0.17.0 milestone

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 23, 2018

Overall my preference would be to remove that compilation flag, as I don't know under what conditions I'd suggest someone disable it.

Recently there was someone with an edge case in the IRC channel with regard to assembly, where they needed to disable it to build. So I think it would be a bad idea to remove it.

Edit: ah this was @MarcoFalke:

2018-11-19 17:05:41     MarcoFalke      Anyone has an idea how to disable compilation of this: crypto/sha256_sse4.cpp:44:9: error: unknown token in expression
2018-11-19 17:05:46     MarcoFalke              "shl    $0x6,%2;"
2018-11-19 17:06:51     wumpus  MarcoFalke: --disable-asm?
2018-11-19 17:06:54     gmaxwell        MarcoFalke: there is a configure option to disable asm
2018-11-19 17:07:09     gmaxwell        lol wumpus' answer was faster an more informative.
2018-11-19 17:07:17     MarcoFalke      I thought it excluded those files, but let me try
2018-11-19 17:07:40     wumpus  although, ideally it *should* detect lack of support for assembly in the compiler in autoconf and do that automatically
2018-11-19 17:08:21     wumpus  there's even been talk of removing disable-asm but I think edge cases such as this make it useful to keep
2018-11-19 17:23:03     MarcoFalke      wumpus: gmaxwell: Thanks, that worked.
2018-11-19 17:23:54     gmaxwell        What strange compiler are you using where it cant compile crypto/sha256_sse4.cpp ?
2018-11-19 17:24:38     MarcoFalke      xenial on travis
2018-11-19 17:24:47     MarcoFalke      gcc5.something that is
2018-11-19 17:25:33     gmaxwell        I wonder if trais has done something weird, becuase that code should compile fine with gcc5.
2018-11-19 17:26:50     MarcoFalke      Its odd, I thought they used gce for the sudo-vms, but it would compile sse41.cpp on gce for me
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.