Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#10396 closed enhancement (fixed)

[PATCHSET] Make Haiku userland compile and work with Clang

Reported by: js Owned by: bonefish
Priority: normal Milestone: R1/beta1
Component: - General Version: R1/Development
Keywords: clang Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

This patch set makes it possible to compile the Haiku userland with Clang and successfully boot and use it.

Change History (43)

comment:1 Changed 6 years ago by js

Has a Patch: set

Changed 6 years ago by js

comment:2 Changed 6 years ago by anevilyak

Type: bugenhancement

comment:3 Changed 6 years ago by stippi

For patch 0001, is there a reason the SetConfigVar line had to be repeated in those Jamfiles? Can't it be done somewhere central? It looks wrong to me to repeat the definition everywhere.

comment:4 Changed 6 years ago by stippi

For 0003, I don't remember the details, but should I be credited in the BStackOrHeapArray? :-P

comment:5 Changed 6 years ago by stippi

All the rest of the patches look fine to me. I only think 0001 could be more elegant.

comment:6 Changed 6 years ago by js

For 0001: As stated in the commit message, neither I nor PulkoMandy found a better way. I'm really looking for a better way, as this is really ugly. If someone knows one, please let me know!

For 0003: You suggested to create such a class, but I wrote it :P.

Last edited 6 years ago by js (previous) (diff)

comment:7 Changed 6 years ago by stippi

Ah, sorry, missed the explanation. Isn't the build system already working with some global variables that are visible from all Jamfiles? Why not add this where global variables are declared? Like even from the configure script. (configure --with-clang=...)

Last edited 6 years ago by stippi (previous) (diff)

comment:8 Changed 6 years ago by js

Well, it should not be global, only for the glibc directory and its subdirectories. That was the problem ;).

comment:9 Changed 6 years ago by stippi

I am applying and testing everything except 0001. Hoping Ingo will comment, as he can most likely think of the proper solution.

comment:10 Changed 6 years ago by stippi

Also, it would already help to just declare the parameters passed to SetConfigVar somewhere global. Like "GLIBC_CLANG_CONFIG_VARS = ..." somewhere global and then in the individual Jamfiles, pass that to SetConfigVar. That way, one could at least change what is passed in one place.

comment:11 Changed 6 years ago by js

Well, that would be yet another workaround. But i'd rather do it right, which means recursively. I'm sure jam can do that and I just haven't figured out how ;).

comment:12 Changed 6 years ago by stippi

Owner: changed from nobody to bonefish
Status: newassigned

I have pushed everything except the first patch, giving up ownership of the ticket. Oh, I realize my "accepting" the ticket didn't work, since I didn't retry after the message that the ticket was changed before I commented... :-)

comment:13 Changed 6 years ago by js

I found a solution for the Jam problem now. I remembered that you could set flags for directories in UserBuildConfig and found it there. I used the same method now. I attached an updated 0001.

I also added 0011 which fixes a ++ that was accidentally added. Thanks to PulkoMandy for noticing this in the diff!

comment:14 Changed 6 years ago by kallisti5

0001 and 0011 applied in hrev46660 as per irc.

comment:15 Changed 6 years ago by kallisti5

Milestone: R1R1/beta1
Resolution: fixed
Status: assignedclosed

marking closed as per js in irc

comment:16 Changed 6 years ago by stippi

Resolution: fixed
Status: closedreopened

There are some problems with BStackOrHeapArray. Re-opening. In any Tracker window, in the size column, the strings are corrupted. I am not yet sure what can cause this, but the string width stuff in app_server is one thing that was touched by the BStackOrHeapArray changes. In particular, it turns out I do hate the Type* operator(), since it makes it non-intuitive for someone like me what actually happens. And it seems that something non-obvious happens with those casts in app_server.

comment:17 Changed 6 years ago by diver

I think the corrupted strings have been fixed in hrev46660.

comment:18 Changed 6 years ago by js

String corruption does indeed sound like the ++ that was accidentally added in glibc by 0006. Can you try hrev46660?

About the Type* operator(): This basically makes sure that you can use it like a normal array, i.e. it gets automatically converted to a pointer, just like a normal array. The idea was that you can easily switch code to BStackOrHeapArray. The only thing that of course will break is sizeof(). But sizeof on an array isn't a good idea anyway, as just switching that array to a pointer will also break it.

Not having that operator would mean code needs to be changed in ugly ways. E.g. if you want to pass a pointer to the data of the array (which seems to be the most common operation), you'd need to change it to &foo[0] instead of foo.

comment:19 Changed 6 years ago by stippi

Oh, I thought I was running the latest version. Probably not. Sorry about the noise!

comment:20 Changed 6 years ago by js

So it does work now and the ticket can be closed? ;)

comment:21 in reply to:  18 Changed 6 years ago by stippi

Replying to js:

About the Type* operator(): This basically makes sure that you can use it like a normal array, i.e. it gets automatically converted to a pointer, just like a normal array. The idea was that you can easily switch code to BStackOrHeapArray. The only thing that of course will break is sizeof(). But sizeof on an array isn't a good idea anyway, as just switching that array to a pointer will also break it.

I understand that much. What wasn't clear to me is how the compiler knew what to do in cases such as these:

BStackOrHeapArray<int32, 64> someInts(arraySize);

SomeFunctionExpectingSizeTPointer((size_t*)&someInts[arrayIndex]);

But Ingo told me that the compiler has to try to convert the object to a pointer first, before it can access it as an array, and since the Type* operator() is the only way to do that, it does indeed do the right thing. Still, it can be non-obvious what happens there, at least it has been for me. What is or isn't ugly code, is really debatable. For example, I would not have considered this ugly code, and this was my original suggestion all the time ago:

BStackOrHeapArray<int32, 64> someIntsArray(arraySize); int32* someInts = someIntsArray.Data();

SomeFunctionExpectingSizeTPointer((size_t*)&someInts[arrayIndex]);

This is just one more line per declaration, but removes BStackOrHeapArray completely from what you need to know about the code that follows. Or in another words, if you could understand the code before the change, you can still understand it just fine, no extra knowledge needed.

(I know that one should not cast an int32 to size_t, because it breaks on x86_64... there are TODOs in app_server about this.)

comment:22 Changed 6 years ago by js

Yes, but that means you need to add that one line of code everywhere where you change it. So it's just more boilerplate and makes diffs to switch code over to BStackOrHeapArray bigger. I fail to see a gain there. Maybe I would if you could provide an example for a situation where it's non-obvious?

comment:23 Changed 6 years ago by stippi

Resolution: fixed
Status: reopenedclosed

Can confirm that the string corruptions are gone.

comment:24 Changed 6 years ago by stippi

I gave you the example in comment:21 and explained what would be gained for me. Please re-read, I don't feel like repeating myself. Now, if that example is obvious to you, or removing having to know about BStackOrHeapArray to understand the code which uses it, is not a gain for you, that is a different thing. We just have to accept that we disagree there.

comment:25 Changed 6 years ago by js

Ok, I thought you meant another example, because the one you gave was pretty obvious to me. The [] operator comes before the & operator (operator precedence). There is no explicit [] operator, so it falls back to the C behaviour, where [] is just *(pointer + offset). As it needs a pointer, it uses the Type* operator(). Then it can evaluate someInts[arrayIndex] and takes the pointer of that and cast it. It all boils down to operator precedence. Would it be more obvious to you if the code were "(size_t*)(&(someInts[arrayIndex]))"? Because that's basically how the compiler treats it due to operator precedence.

comment:26 Changed 6 years ago by stippi

No, problem, but you still don't quite understand what I mean. You don't need to explain to me about operator precedence, although that stuff is generally more fuzzy to me than other areas. However, I wrote that code, so as far as that is concerned, I understand it.

I am talking about one new thing introduced by BStackOrHeapArray that one needs to understand here, which is that before the compiler can use the array operator, it needs to apply a pointer cast. If one knows that, it becomes clear why the compiler knows how to do the right thing in my example, and why a [] operator is not needed in BStackOrHeapArray.

So the gain for me is not having to know an additional thing. Obviuosly I knew that my solution would require one more line to be written, I even pointed it out in that comment. So you don't need to remind me of that, but you need to ask yourself why I am Ok with it. The answer is simply that it is slightly more important to me not needing to know one more thing about the code than having to write one more line. That's all. I don't think "ugly" has anything to do with it. That is like thinking code needs to be short above anything else, which is certainly not true as simple as that.

comment:27 Changed 6 years ago by js

Oh, I see what you mean. So what you is that it might be confusing that Type* operator() is used for [], because it might not be obvious enough. What about adding an operator[] then? Then it's clear, isn't it? It would be useless code, but make it clear. Or we could add a comment explaining it. I think both would make it equally clear, but the first would introduce unnecessary code overhead.

comment:28 Changed 6 years ago by stippi

When I still thought there was something broken, I actually tried to add the operator[]. If memory serves, I tried adding this:

Type& operator[] (int const& index) {

return fData[index];

}

const Type& operator[] (int const& index) const {

return fData[index];

}

But then I got compiler errors about some ambitious conversions, specifically in that archiving code, but I don't know if there would have been more. So maybe the best option could be the comment. Or maybe just do nothing, I just tried to explain what benefit I would personally see with not using any operator at all and just having one more line to type. But I may not be a good measure for these things. Maybe it is obvious to most other people why the pointer operator is enough. After all, when using the class, one should look in the header...

BTW, when googling this stuff, I also read that Type* operator() is usually declared const. It makes sense to me. Do you think that should be ammended in the header?

comment:29 Changed 6 years ago by js

The comment is definitely something we should add. It confused you, so it might confuse others as well.

const Type* operator() has a problem: It would mean the data the pointer points to is read-only. But if you want to modify the data in the array, this would be a problem.

comment:30 Changed 6 years ago by js

Has a Patch: unset

comment:31 Changed 6 years ago by pulkomandy

I think stippi meant operator Type*() const ?

You have to be careful with const, it can mean a lot of things :)

comment:32 Changed 6 years ago by js

Ah, I see. I think that const might not be correct, as we return a non-const pointer and thus the internal state could be changed. A const Type* operator() const OTOH could work.

Note: See TracTickets for help on using tickets.