[PATCHSET] Port kernel, bootloader and runtime_loader to Clang.

[js]

The attached patches make it possible to fully build Haiku with Clang. The resulting Haiku does boot, however, there is one problem left: If SMP is enabled, the bootloader seems to freeze. The icons appear, yet the first icon is never lighted up. This is definitely a boot loader problem and not a kernel problem, as using a Clang-built bootloader with a GCC-built kernel has the same problem.

[pdziepak]

As for patch 5, /src/system/kernel/util/kernel_cpp.cpp seems to be much better place for defining such function. Also, the comment is not true.

[js]

Hm, yes, maybe it could be moved there.

What exactly is not correct about the comment? GCC did not emit references to atexit, after all.

[pdziepak]

It is not true that GCC doesn't call correctly destructors of static objects. It is using __cxa_atexit to register appropriate destructors. This (together with __cxa_finalize) allows static objects in dynamically loaded libraries to be destroyed when the library is unloaded.

[js]

Well, I did not find cxa_atexit in the bootloader or runtime_loader. So, as it does not exist there, I guess GCC does not really emit references to it, right?

Anyway, I noticed that I can tell Clang to also use cxa_atexit. Maybe the better way would be to tell it to use cxa_atexit and add that everywhere? It seems there is one cxa_atexit in the kernel, but not in the bootloader or runtime_loader - but those would also need it.

[pdziepak]

__cxa_atexit is defined in /src/system/kernel/util/kernel_cpp.cpp which is shared by kernel, bootloader and runtime loader. Therefore, all of them have the function defined, as I said it is an essential part of C++ ABI that GCC uses.

If the Clang can be set to use __cxa_atexit instead of atexit that seems to be the best solution.

[korli]

As for patch 3, please drop the static keyword (C++) or use a define.

As for patch 6, you should define dso_handle to some address (ie (void*) &dso_handle).

Patches 1, 2 and 4 look OK.

[js]

Dropping static from patch 3 would obviously cause linking errors, as every file including that .h would then emit that symbol. The static is very important here. It could be changed to a define, but the difference would only be that it is not typed anymore.

As for patch 6, it seems __dso_handle does not need to have a value because only its address is used (see also ​http://www.lowlevel.eu/wiki/C%2B%2B and ​http://wiki.osdev.org/C%2B%2B). Initializing it would also move it from section .bss to section .data - that is not really desirable.

[korli]

Replying to js:

Dropping static from patch 3 would obviously cause linking errors, as every file including that .h would then emit that symbol. The static is very important here. It could be changed to a define, but the difference would only be that it is not typed anymore.

I disagree, it's C++, not C.

As for patch 6, it seems __dso_handle does not need to have a value because only its address is used (see also ​http://www.lowlevel.eu/wiki/C%2B%2B and ​http://wiki.osdev.org/C%2B%2B). Initializing it would also move it from section .bss to section .data - that is not really desirable.

libgcc defines the variable ​here. Today runtime_loader doesn't have this symbol, but other loadable objects have it in the data section. runtime_loader can probably be seen as a main program, so setting to zero should be fine.

[js]

Oh, so C++ automatically defines it as static if it's const? Anyway, adding that static there does not seem to hurt, I've created a little test case and it works.

As for setting __dso_handle to 0: That woud mean it's in section .data and not .bss. Both links I gave seem to indicate that it's fine in section .bss. Why do you think it's necessary to move it to section .data?

[korli]

Replying to js:

Oh, so C++ automatically defines it as static if it's const? Anyway, adding that static there does not seem to hurt, I've created a little test case and it works.

Hmm, why adding a useless keyword then?

As for setting __dso_handle to 0: That woud mean it's in section .data and not .bss. Both links I gave seem to indicate that it's fine in section .bss. Why do you think it's necessary to move it to section .data?

It's not necessary indeed for non-shared objects, it will just be ignored, as if defined to zero.

[js]

Hm, to make it more clear to people like me who are more at home with C than with C++ and sometimes forget about such things? ;) Nah, I can remove it, no problem.

About the dso handle, I think we can just keep it this way then.

About 0004: Clang complains about it with [], but accepts [0], which is strange. [0] should be equally wrong in C++. Maybe it accepts it as [0] is a GNU extension? The correct way would be to use [1], but that would break memory management. I'm not sure about what to do with it. Just change it to [0] and be done, even though it's still not proper C++? It seems to be wrong anyway, because memory allocation will be wrong, even with []. So I think this code is wrong by design and needs to be fixed properly some day. What do others think?

Other than that, can the patches be applied after removing the useless static?

[korli]

Applied all patches but number five in hrev46669. I let you see what remains.

[pdziepak]

Replying to js:

About 0004: Clang complains about it with [], but accepts [0], which is strange. [0] should be equally wrong in C++. Maybe it accepts it as [0] is a GNU extension? The correct way would be to use [1], but that would break memory management. I'm not sure about what to do with it. Just change it to [0] and be done, even though it's still not proper C++? It seems to be wrong anyway, because memory allocation will be wrong, even with []. So I think this code is wrong by design and needs to be fixed properly some day. What do others think?

options[1] definitely wouldn't be a correct solution. If we really wanted to be 100% standard compliant we should remove that member at all, but that really doesn't look like a good idea. options[0] seems fine, C++ doesn't allow flexible array members and there isn't much we can do about it.

I really don't see how that flexible array member makes that code "wrong by design" or breaks memory management.

[pdziepak]

Replying to korli:

Replying to js:

As for setting __dso_handle to 0: That woud mean it's in section .data and not .bss. Both links I gave seem to indicate that it's fine in section .bss. Why do you think it's necessary to move it to section .data?

It's not necessary indeed for non-shared objects, it will just be ignored, as if defined to zero.

Runtime loader is also a shared object. However, it doesn't really matter what is the value of __dso_handle, only its address is important.

BTW I don't really see why we would need to explicitly set __dso_handle to zero or how such explicit initialization would move it from .bss to .data. Static and global variables are by default initialized to zero.

[js]

You are right, the compiler might be clever enough to know that everything in section .bss gets set to 0 anyway, at least Clang does. However, any value other than 0 would definitely move it to section .data (and for some compilers, I could imagine setting it to 0 would do the same).

[waddlesplash]

Clang support has been merged to master.