gcc wrongly optimizes NULL check in strdup.

[pulkomandy]

This was explored in length in this ML thread: ​http://www.freelists.org/post/haiku-development/Aw-Re-hrev45320-Yet-another-nonobvious-effect-of-ftreevrp-optimization

Well, for some reason the problem happens again here. Web+ will crash when trying to play a youtube video, for example.

I think this started happening somewhere between hrev47163 and hrev47184, but it could be a bit earlier, as I think the ptrdiff_t change in 47184 led to a rebuild of libroot, including the strdup function. I'm building from Haiku, using gcc_x86-4.8.2_2014_03_20-1-x86_gcc2.hpkg, in case this makes a difference. Didn't test if nightlies are affected.

[korli]

Have you tried -fno-builtin-strdup?

[pulkomandy]

I just did. Adding "-fno-builtin-strdup -fno-builtin-strndup" to the C++FLAGS in src/system/libroot/posix/string/Jamfile fixes the problem of badly generating our function.

We still risk hitting the problem again if gcc decides to actually use the builtin, so maybe it's better to add this to our global flags.

[korli]

Weird, did you actually check the assembler for the missing null check? Also did you check that -fno-treevrp is actually used when building libroot? Dunno it could be an hybrid problem.

[pdziepak]

With builtins enabled GCC is free to replace any call to strdup() with its own code. This enables it to assume that NULL is never passed to that function (POSIX doesn't allow it) and remove any redundant NULL checks. Using -fno-treevrp only fixes symptoms of the problem (an optimization that uses, wrong in our case, assumption that pointers passed to strdup are never NULL) but the cause remains (the assumption itself).

Personally, I think that in our code we should not pass NULL to strdup() at all and eventually remove the check for it in GCC4 builds since it is often an unnecessary performance cost. That may cause problems with non POSIX compliant applications, though.

[pulkomandy]

I understood why the issue popped up for me: I didn't re-run configure following the clang support changes. So HAIKU_CC_IS_CLANG is not properly set here, and there is a guard preventing -fno-tree-vrp to be added to the flags.

Anyway, shouldn't we enable tree-vrp and disable the built-in strdup instead? It sounds like things would crash if they use the builtin, no matter if the optimization is enabled.

I don't see anything in the POSIX spec that explicitly disallows handling NULL pointers. There's the possible performance problem, still, which may be a valid reason for omitting it...

[bonefish]

Replying to pulkomandy:

I don't see anything in the POSIX spec that explicitly disallows handling NULL pointers.

It is undefined, so the implementations are free to gracefully handle a NULL pointer or not. Compliant applications cannot rely on either behavior.

In this case there are two implementations, ours and GCC's (and possibly that of other compilers). I think the best solution is to fix the code that relies on NULL being an acceptable argument. Otherwise:

• The likely faster intrinsic would have to be disabled.
• We'd have to take care that the intrinsic is disabled in all compilers, which is likely to be a trial and error (=crash) process, since the odds are that a future porter of a different compiler won't think of that.
• The code wouldn't be portable, which may eventually bite the author. Respectively the author will get used to that specific behavior and erroneously rely on it on other platforms as well.

[axeld]

I don't see much room for changes for our GCC 2 ABI; since BeOS accepted NULL, so we have to.

I usually prefer to handle NULL pointers gracefully, but being able to use intrinsics is definitely worth a thought for GCC4/Clang.

[bonefish]

I think that having our strdup() implementation check the argument for NULL is just fine. However, we should still fix our code (and third party open source code) not to rely on that.

[waddlesplash]

This was fixed by re-enabling tree-vrp and disabling deletion of null-pointer checks.

[nielx]

Assign tickets with status=closed and resolution=fixed within the R1/beta2 development window to the R1/beta2 Milestone