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 or de-UB --release from the CLI #4709

Open
schveiguy opened this issue Jul 18, 2024 · 13 comments
Open

remove or de-UB --release from the CLI #4709

schveiguy opened this issue Jul 18, 2024 · 13 comments

Comments

@schveiguy
Copy link
Contributor

schveiguy commented Jul 18, 2024

See my post here: https://forum.dlang.org/post/pkeasakdbugctqdyetnw@forum.dlang.org

Just, remove the --release flag. It's completely useless, and does nothing to win benchmarks.

In fact, we pretty much "won" a benchmark here without -release: https://github.com/jinyus/related_post_gen/blob/main/d_v2/dub.json

The main drawback of the --release flag is that it seems to suggest when you release your D code you should use it. This is completely wrong.

Alternatively, we could alias --release to mean -O3, to avoid removing the UB that comes with removing bounds checks and asserts.

There is nothing that says we have to copy DMD's mistakes...

@quickfur
Copy link

quickfur commented Jul 18, 2024

+1, kill it with fire. It's not only useless, but harmful to code that relies on well-defined behaviour. Let dmd continue with whatever mistakes it wants, we can do better with LDC.

@schveiguy
Copy link
Contributor Author

schveiguy commented Jul 18, 2024

Alternatively, make --release an alias to -O3 and not remove asserts. In fact, this might be the better choice... It will fix the problem without breaking anyone's build!

@schveiguy schveiguy changed the title remove --release from the CLI remove or de-UB --release from the CLI Jul 18, 2024
@kinke
Copy link
Member

kinke commented Jul 18, 2024

I'm not convinced - it's the analogon to C's -DNDEBUG to disable asserts (incl. bounds checks etc.) by default. Try an LDC build with enabled assertions (incl. LLVM assertions), it's way slower than a 'release' :D build.

@p0nce
Copy link
Contributor

p0nce commented Jul 18, 2024

I don't quite understand the vendetta against -release, sure the name isn't well choosen, but it's been explained and working for years. In particular, what's in it for the user? One less flag that must be replaced by many many flags.

@kinke
Copy link
Member

kinke commented Jul 18, 2024

Yeah agreed, the name surely isn't ideal. But it's been there for ages, so at least for backwards compatibility, it would need to remain for quite a while anyway.

Dub using it by default for all release{,-debug,-nobounds} build types is something that might be worth discussing. And note the release-nobounds there, explicitly disabling bounds checks in @safe functions too, for the 'ultimate performance'. ;)

@schveiguy
Copy link
Contributor Author

the name isn't well choosen

This is the biggest problem. People think they should use this for when they release their project. But there is no requirement to release with all asserts turned off. Especially bounds checks.

I'd also be fine with a rename to something like --nosafety or something like that.

Dub using it by default for all release{,-debug,-nobounds} build types is something that might be worth discussing

This is also a valid path. Just something where people can't accidentally trip on this footgun.

Another further option is to still remove some things (invariant calls are probably not something you want to always have on), but keep bounds checks and asserts.

It should just not be easy to turn all the safeties off. You should have to work at it, and understand what you are disabling.

On the subject of dub, I think also it should be possible to build dependencies with different options for safety than the main project, which you might want to keep all checks.

it's the analogon to C's -DNDEBUG

That's a terrible name also, but not as inviting as release. If I didn't know it is the analog, NDEBUG sounds like it is probably enabling some kind of debugging.

@schveiguy
Copy link
Contributor Author

I don't quite understand the vendetta against -release

This comes from helping many people on discord/d.learn who think they should use this switch to release their code, then have issues with UB.

As an example, one user said they "didn't know why" but their project only worked in release mode. Why? Because it was corrupting memory, and not crashing right away!

@JohanEngelen
Copy link
Member

I don't quite understand the vendetta against -release

This comes from helping many people on discord/d.learn who think they should use this switch to release their code, then have issues with UB.

I wonder where they get that from. You have to actively search for -release to find it. The documentation says: "--release - Compile release version, defaulting to disabled asserts/contracts/invariants, and bounds checks in @safe functions only".

We could remove "Compile release version" because that has no unambiguous meaning, but otherwise I think we should keep it.
I don't even know what it would mean to have a switch needed to "release" code, and I don't think D should be tailored to people who do think they know ;-)

Similarly, -O will make aggressive use of any UB, which is probably also not expected by some.

@schveiguy
Copy link
Contributor Author

schveiguy commented Jul 18, 2024

I wonder where they get that from.

https://learn.microsoft.com/en-us/visualstudio/ide/understanding-build-configurations?view=vs-2022

image

In other words, this is standard terminology in most IDEs

@0xEAB
Copy link

0xEAB commented Jul 18, 2024

I'd also be fine with a rename to something like --nosafety or something like that.

How about --release-benchmark? (or --release-footgun? /s)

@0xEAB
Copy link

0xEAB commented Jul 18, 2024

In other words, this is standard terminology in most IDEs

Yes. And it is not unique to IDEs.

@p0nce
Copy link
Contributor

p0nce commented Jul 18, 2024

How about having dub -b release keep bounds checks then? (assertions being a separate debate maybe)
Those of us that want maximum speed already have to use dub -b release-nobounds

@kinke
Copy link
Member

kinke commented Sep 1, 2024

I forgot to post that CMake Release builds default to -DNDEBUG; this might be one reason why I don't have any problem with -release.

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

No branches or pull requests

6 participants