This is the more typical approach used in the tree for system calls
with per-ABI structure layouts.
Obtained from: CheriBSD
Sponsored by: AFRL, DARPA
Details
Details
- Reviewers
olce brooks - Group Reviewers
cheri - Commits
- rG52864a411a6f: setcred: Move initial copyin of struct setcred out to per-ABI syscall
rG9a5b64536743: setcred: Move initial copyin of struct setcred out to per-ABI syscall
rG6292eecfd95c: setcred: Move initial copyin of struct setcred out to per-ABI syscall
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 68807 Build 65690: arc lint + arc unit
Event Timeline
| sys/kern/kern_prot.c | ||
|---|---|---|
| 573 | We can't, it gets modified by the function? Oh, you mean the pointer? I mean, we don't generally use that annotation in the kernel (const on pointer values vs const on what the pointer points to), and you can't really change the caller's pointer anyway as it is passed by value. That just means the function promises not to move the pointer I guess? | |
| sys/kern/kern_prot.c | ||
|---|---|---|
| 573 | Yes, the pointer, and yes, as you say, this only means the function promises not to change the pointer itself. Of course, that's a very small improvement that has low but non-zero value, so while it would be silly to impose that as a style matter, I'd like that at least we can still add such annotations while modifying code and keep those that are already in place. | |
| sys/kern/kern_prot.c | ||
|---|---|---|
| 573 | I would have guessed that this had the effect of making the pointer not an array, but that doesn't seem to be true. https://godbolt.org/z/4a3nda6MP In the linked example, i++ is prevented, but you can still access any offset from the pointer so it isn't really doing anything useful. | |
| sys/kern/kern_prot.c | ||
|---|---|---|
| 573 |
It still prevents accidentally changing the pointer itself, via, e.g., = where == is meant, even in cases where no warnings would be emitted, and that is useful. | |
| sys/kern/kern_prot.c | ||
|---|---|---|
| 573 | Modern compilers shout at you out of the box if you do that. Putting const everywhere gets messy very quickly, please don't. | |
| sys/kern/kern_prot.c | ||
|---|---|---|
| 573 |
No, unfortunately, they don't, including in very simple cases (I've tried it, with GCC and clang, -Wall, with and without optimizations, *all* cases fail). And even if they would, that's only one example of mistakes one can do. A variable name mistake in a simple assignment can never be caught. And so on.
This sentence most probably implies that you've been at some point in a hard-to-entangle scenario with lots of const. I can sympathize, but what you experienced simply has nothing to do with what's happening here. Here, there's no const added to some type or field reused elsewhere. Here, there's not even a const to pointed data. There is instead and only a const to a function argument, which by nature is completely local to the function and just an internal contract of it which never propagates to anything. And it's just a single const, not something put everywhere. It would be great if you could overcome your general const detestation and recognize its usefulness (sometimes big, sometimes tiny) in some situations. Now, is keeping a local annotation that was added deliberately, represents a still true assumption and that doesn't cast a shadow to anything else really too much to ask? | |
| sys/kern/kern_prot.c | ||
|---|---|---|
| 573 | [jrtc27@ref16-amd64 ~]$ echo 'int foo(void *p) { if (p = 0) return 1; return 0; }' | clang -x c - -fsyntax-only
<stdin>:1:26: warning: using the result of an assignment as a condition without parentheses [-Wparentheses]
1 | int foo(void *p) { if (p = 0) return 1; return 0; }
| ~~^~~
<stdin>:1:26: note: place parentheses around the assignment to silence this warning
1 | int foo(void *p) { if (p = 0) return 1; return 0; }
| ^
| ( )
<stdin>:1:26: note: use '==' to turn this assignment into an equality comparison
1 | int foo(void *p) { if (p = 0) return 1; return 0; }
| ^
| ==
1 warning generated. | |
| sys/kern/kern_prot.c | ||
|---|---|---|
| 573 | You need to play a tiny bit more. $ echo 'int foo(int a, int b) { int c = a = b; return (c); }' | clang -x c - -fsyntax-only $ Oh, and also test it with multiple versions of clang and GCC. | |
| sys/kern/kern_prot.c | ||
|---|---|---|
| 573 | Strictly speaking, I think Jess was referring to your earlier example of using = where == is intended which is the thing modern compilers warn about. They certainly don't warn about assignments that are just to the wrong variable. I still think that overall this style is unusual in FreeBSD currently. | |
Comment Actions
Thank you for listening.
| sys/kern/kern_prot.c | ||
|---|---|---|
| 573 | The sub-point of mine Jessica jumped on is that modern compilers are still incapable of detecting = instead of == mistakes even in simple cases. And I stand by it simply because it's true. Of course, I'm perfectly aware that compilers do detect the canonical mistake if (a = b). I've just given a very example of a simple case where the compiler doesn't report anything, despite what you seem to think: The intended correct code was int c = a == b; instead of int c = a = b;. I stumbled on several other such situations in the course of the past years, I don't remember them right now but it shouldn't be too hard to find other cases where the compiler says nothing. My initial message has more points though. One of which is that a compiler will never be able to catch an assignment to a wrong variable that has the same type. const will. It wouldn't be the first time we change style for good in FreeBSD. I'm not even asking for adding const everywhere, just that people who want to use it can do it liberally especially in places where its effect stays purely local. | |