search

ldc-developers / ldc

The LLVM-based D Compiler.
http://wiki.dlang.org/LDC
Other
1.22k stars 262 forks source link

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

Open schveiguy opened 4 months ago

schveiguy commented 4 months ago

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 commented 4 months ago

+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 commented 4 months ago

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!

kinke commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

In other words, this is standard terminology in most IDEs

Yes. And it is not unique to IDEs.

p0nce commented 4 months ago

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 commented 2 months ago

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.