Opened 6 years ago

Closed 23 months ago

Last modified 5 months ago

#10803 closed bug (fixed)

gcc wrongly optimizes NULL check in strdup.

Reported by: pulkomandy Owned by: nobody
Priority: normal Milestone: R1/beta2
Component: System/ Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Platform: All


This was explored in length in this ML thread:

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.

Change History (11)

comment:1 by korli, 6 years ago

Have you tried -fno-builtin-strdup?

comment:2 by pulkomandy, 6 years ago

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.

comment:3 by korli, 6 years ago

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.

comment:4 by pdziepak, 6 years ago

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.

comment:5 by pulkomandy, 6 years ago

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...

in reply to:  5 comment:6 by bonefish, 6 years ago

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.

comment:7 by axeld, 6 years ago

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.

comment:8 by bonefish, 6 years ago

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.

comment:9 by axeld, 3 years ago

Owner: changed from axeld to nobody
Status: newassigned

comment:10 by waddlesplash, 23 months ago

Resolution: fixed
Status: assignedclosed

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

comment:11 by nielx, 5 months ago

Milestone: R1R1/beta2

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

Note: See TracTickets for help on using tickets.