[PATCH] Make Haiku build on OS X Mavericks

[js]

Attached is a set of patches that make Haiku build on OS X Mavericks.

[pulkomandy]

Applied patches 1-5 in hrev46551. I'm not sure about patch 6, I think this function alias is useless, and the few places that call it could use the other name for the function.

Is there a good reason to keep it?

[js]

I'm not sure where/if this is used. I did not try it because even if the host tools would have built then, it's possible that it's used in other places where it is used, possibly even 3rd-party code? The weak alias was giving problems on OS X, so I decided to just jump into the other. A smart compiler might even make that an alias on platforms where it is supported, so I think there is no real loss here. And even if it does not do that, GCC can definitely do tail-recursion optimization and thus that function would get a single jmp, so the loss is about 5 bytes.

[pdziepak]

The sixth patch is definitely wrong. The new public symbol delete_driver_settings was intentionally made weak so that it would not causes any problems when a legacy binary also defines it.

I am also worried a bit about the first patch. We generally should be very careful when using any compiler built-ins. Some of our functions differ from what is defined by the standard (e.g. our strdup() accepts NULL pointer). Using built-ins not only changes the behaviour of the function but also may allow compiler to make some incorrect optimizations (we have already had such problem during our unpleasant adventures with -ftree-vrp). Besides, AFAIK when cross compiling -fno-builtin is set, so the compiler shouldn't use its own versions of strl{cpy,cat}() anyway.

[pulkomandy]

The first patch could be modified to #undef the two functions and declare the prototypes instead, then.

[js]

Your worries about the builtins are not necessary. The file in question is used in two modes: On the host OS and on Haiku.

On Haiku, strlcpy and strlcat aren't defines, so the builtin is not used. On other systems, e.g. OS X Mavericks, string.h defines these to Clang's builtins.

So this really is not dangerous at all and -fno-builtin is just meaningless, because not the compiler defines it to the builtins, but the libc.

Worrying about different behaviour of builtins is also not important here, as they are from the OS anyway if available.

For the delete_driver_settings part, we should just remove it then I guess.

[pdziepak]

Oh, right. I was under the impression that it is the compiler that causes problems not libc. If it's libc then the patch looks good.

As far as I understand the problem with delete_driver_settings is because we are using it in a code compiled for a host. Since we have quite a lot of weak aliases defined in libroot removing it will only defer the problem to a moment any of the remaining functions with weak aliases is used in a host targeted code. Perhaps a better solution would be to modify B_DEFINE_WEAK_ALIAS so that it doesn't use weak aliases when building for a host, in such case we don't have to worry about ABI anyway.

[js]

I attached another patch. This makes bfs_shell work for me when invoked manually, however, I did not get it working inside the build system yet. That might be a problem on my end, though, as I did not have it working at all on this machine, so an error in my UserBuildConfig is possible.

[js]

It was indeed a problem with my UserBuildConfig. Accidentally, I had SetUpdateHaikuImageOnly 1 in it.

With all patches from this ticket applied, I successfully built Haiku on OS X Mavericks and also successfully booted it.

What I also should mention: Patch 7 will likely fix the bug that I could not boot the kernel when compiled with Clang. I got the exact same problem back then, with the only difference being that it was in the kernel space instead of the user space. So this way, I could debug a kernel bug in userland, which was convenient :).

And what do we do with patch 6 now?

[umccullough]

FWIW, the modification in patch #7 essentially resolves CID 601132 (marked as false positive) and 991608 (marked as bug) referring to the potential uninitialized use of vnode (even though it wasn't actually used as such).

[bonefish]

I'm pretty sure the weak alias wasn't meant to avoid problems with legacy apps (cf. ​http://cgit.haiku-os.org/haiku/commit/?id=9d6e5fdb651066357cf4e24f10c42238ad08d0b7). One way or another there wouldn't be any problems anyway (respectively they wouldn't differ). I think the patch is OK.

[js]

Can someone (maybe you, umccullough?) apply patch 7 then please? I think this is a pretty clear thing. It was undefined behaviour, even fixes 2 CIDs, so there really should be no hesitation in committing it.

Also, could patch 6 be committed, please? I think bonefish showed that it will not cause trouble.

[js]

I just attached another patch.

It turns out that agg is used for some icon tools on the host system. I did not realize this is needed because my Mavericks changes were based on my Clang branch. So patch 8 I attached now is actually just copied from my Clang branch, which means it has already been reviewed on the mailing list.

[axeld]

Regarding patch 8: I wonder why m_profile is const in the first place, but well.

In any case thanks for the patches. They have been applied in hrev46558 and hrev46559 (due to the late patch 8).

[js]

Thanks for committing these, and sorry for adding patch 8 in the last minute ;). I only noticed that this is required when someone on IRC said it did not work for him.