sflist: SYS_SFLIST_FLAGS_MASK must be a long not an int#17025
sflist: SYS_SFLIST_FLAGS_MASK must be a long not an int#17025nashif merged 1 commit intozephyrproject-rtos:masterfrom
Conversation
|
For the record, the |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
I'm not trying to be obstructionist here but I have a vested interest in the LP64 64-bit implementation, so I just want to be sure we get all of these off on the right foot .. some kind of guidelines about the generalization of int/pointer conversions etc.
|
On a technical level, "long" is going to do the right thing on ILP32 and LP64, which are all the toolchains anyone in this industry cares about support, have supported or will support at any time +/- two decades. That said, we do have a mandate from the MISRA work to use less variant types, like u32_t. Things like size_t/ssize_t/ptrdiff_t/intptr_t/uintptr_t are not invariantly-sized types, of course. But they probably look more kosher to tools that are going to whine about a "long". I'd say we should pick one of those, but really don't care which. |
|
@andyross It may be that |
|
@andyross Not to mention that something tells me that less than a decade ago, somewhat said "ILP32 is the only mode Zephyr will ever care about", and that's how we ended up with this cleanup to begin with. :) |
|
On Mon, 24 Jun 2019, Charles E. Youse wrote:
It may be that `long` does the right thing for our circumstances, but
in that case, why are we ever using `intptr_t` etc. elsewhere?
IMO, this is more about semantic than anything else. If you know that
your integer value always represents a memory address then intptr_t
might carry some extra meaning. Otherwise a long is what people use in
practice.
See `POINTER_TO_INT`
I've used them in the context of thread argument conversion mainly
because you cannot cast an int to a void* without going through an
intermediate type such as long or intptr_t. So the INT_TO_POINTER()
looks nicer than a double cast. And POINTER_TO_INT() is just balancing
things out for consistency.
We're in the first
phase of changes for 64-bit cleanliness, I think we should pick a
consistent standard. If that means assuming `sizeof(long) ==
sizeof(void *)` then that's fine, but let's just assume that then.
Absolutely. I have a pending PR enforcing that at all levels already.
|
If the integer is the correct size, there's no need for an intermediate cast, right? Under what circumstance do we need to cast an incorrectly-sized integer to a pointer? There's almost no reason to ever do that, and if the need arises, I say we make the programmer double-cast to make the pun obvious. |
I understand what you're saying, but I would say in circumstances where an integer value always represents a memory address, just use a |
|
On Mon, 24 Jun 2019, Charles E. Youse wrote:
I understand what you're saying, but I would say in circumstances where an integer value always represents a memory address, just use a `void *`.
Sure. Problem is when you need to perform arithmetics on that value.
It's nicer not to have to cast it all the time.
If we're in agreement that `long` and pointer are always the same size, then let's just dispense entirely with `intptr_t` et al. (I've never liked them anyway.)
You won't find any objection from me there. But I'm not the one to make
that call.
|
Well, you have PRs in that use
Well, I think we'll both agree just to call it |
|
On Mon, 24 Jun 2019, Charles E. Youse wrote:
If the integer is the correct size, there's no need for an intermediate cast, right?
Thread functions only accepts 3 void* arguments.
If you want to pass an int, or an int32_t for that matter, to a thread,
then you have to cast it to a void* for the thread creation API, and
cast it back to an int in the thread function.
Under what circumstance do we need to cast an incorrectly-sized integer to a pointer?
On 64-bit targets, you cannot take an int or an int32_t and cast it to a
void* without making the compiler complain at you. The only way is to
first cast the int to a long (or intptr_t -- here the semantic is more
obvious) and then to a void*. And because this is rather ugly, and
because many people are likely to ignore this subtlety when writing
their app, I think it is best to get into the habit of using
INT_TO_POINTER() with such interfaces where arguments have to be void*
so it is 64-bit clean upfront without having to know why that double
cast is needed in the first place.
That sayd, I don't think INT_TO_POINTER() should be used everywhere.
Sometimes as you say it is best to make the intent clear and
unabstracted. It really depends on the context and the semantic you want
the code to have.
|
|
On Mon, 24 Jun 2019, Charles E. Youse wrote:
> You won't find any objection from me there. But I'm not the one to make that call.
Well, you have PRs in that use `intptr_t` instead of `long`. So are making the call, so to speak.
Yes, because at some point I was asked to use intptr_t instead. Now I'm
asked to remove them. :-)
When #16645 finally gets merged, I think it would make sense to just say
officially that any pointer that has to be treated as an integer should
use an unsigned long type. But then people might claim that uintptr_t is
shorter. In practice they're (will be) equivalent so really I don't know
if this is worth enforcing only one of them.
|
We've agreed on this many times. I'm not sure who's been asking you to use
We can't enforce what people do in the apps they make for Zephyr, but we can enforce coding standards in the project codebase. And, seriously, if people think that typing a few extra characters is too much work .... I really don't even know what to say to that. |
When splitting the pointer from the flag, ~SYS_SFLIST_FLAGS_MASK remains a 32-bit value because of the lack of an L qualifier. Let's qualify it with UL so the top half of 64-bit pointers is not truncated. Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
|
On Mon, 24 Jun 2019, Charles E. Youse wrote:
We can't enforce what people do in the apps they make for Zephyr, but we can enforce coding standards in the project codebase. And, seriously, if people think that typing a few extra characters is too much work .... I really don't even know what to say to that.
That's all fine with me.
I won't be the one submitting a global s/uintptr_t/unsigned long/g though!
|
Yes, I know why we're having this conversation :-)
If your argument really is that |
Hah, ok. Maybe I'll work up the chutzpah to do it, then. I understand that this is probably very frustrating for you, and I'm sorry to barge in on your work like this, but I think it's important that we set a standard sooner rather than later. (Ordinarily I wouldn't be so nitpicky but I've got a personal stake in 64-bit Zephyr.) |
|
On Mon, 24 Jun 2019, Charles E. Youse wrote:
I understand that this is probably very frustrating for you, and I'm sorry to barge in on your work like this, but I think it's important that we set a standard sooner rather than later.
I agree. That's the purpose of #16645 by the way, which could also
benefit from your review.
(Ordinarily I wouldn't be so nitpicky but I've got a personal stake in 64-bit Zephyr.)
FYI, I have it working fine here already. I'm only a few more generic
patches away from submitting native_posix_64 support.
|
@andyross @gnuless @npitre Who, when, where? |
|
On Tue, 25 Jun 2019, Alberto Escolar wrote:
>> any pointer that has to be treated as an integer should use an unsigned long type.
>
> We've agreed on this many times.
@andyross @gnuless @npitre Who, when, where?
I don't know. I sense some discussions prior my involvement here.
(silly things apart) What is the problem with `intptr_t`?
Its color?
I would not be surprised if, sometime soon, somebody would try to make a native_win port. And there, long is 32 bits both for win32 and win64.
Maybe the best solution in that case would still consist of enforcing
a long to be 64 bits for consistency, like `#define long DWORD` or the like.
But I'd recommend not to overdesign here until it is actually needed, if ever.
|
@andyross @gnuless @npitre : Wouldn't it be good enough to use |
|
On Tue, 25 Jun 2019, Alberto Escolar wrote:
@andyross @gnuless @npitre : Wouldn't it be good enough to use `void *`, in general, (as unknown pointer type), and cast it to `uintptr_t` when needed (instead of long)?
Sure. Anyway, it seems that the preference is still intptr_t over long.
But having consistency i.e. intptr_t is always equivalent to long, makes
it much convenient with printf than the obnoxious PRIiPTR thing.
|
5 participants
When splitting the pointer from the flag, ~SYS_SFLIST_FLAGS_MASK remains
a 32-bit value because of the U qualifier. Let's qualify it with UL so
the top half of 64-bit pointers is not truncated.