The sparse C compile checker has a set of extensions to the gcc type attribute set: __attribute__((noderef)) __attribute__((address_space(N))) ... where N is an arbitrary integer. sparse will warn if pointers tagged with different address spaces are mixed, and if a pointer tagged with "noderef" is dereferenced. It would be highly useful if these protections could be extended to gcc itself and thus get more ubiquitously run. The main use case, of course, is the Linux kernel, but almost any low-level application which has to deal with multiple "manually maintained" address spaces is likely to benefit. __attribute__((force)) ... usually used as part of a cast, is used to suppress the warning.
A few additional behavior notes: Casting a pointer from one address space to another must produce a warning, unless the type used for the cast includes __attribute__((force)). (Typically, a codebase will have safe ways to perform such conversions, wrapped up in functions with appropriate parameter and return types, with a force'd cast in them; for instance, the kernel's copy_to_user and copy_from_user functions.) __attribute__((force)) can be used in the type of a function parameter as well, in which case the caller can pass a pointer to a different address space without complaint. Dereferences of a noderef type inside a sizeof() or typeof() must not generate warnings. For instance: __attribute__((address_space(1),noderef)) int *a; typeof(*a) b; /* b has type "int" */ typeof(*a) *c; /* c has type "int *" */
Here's a test case from Sparse, demonstrating some tricky behavior of noderef: # define __A __attribute__((noderef)) struct x { int a; int b; }; struct y { int a[2]; }; static void h(void) { char __A *p; char __A * * q1; char * __A * q2; struct x __A *xp; struct x __A x; int __A *q; int __A *r; struct y __A *py; q1 = &p; q2 = &p; /* This should complain */ r = &*q; r = q; r = &*(q+1); /* This should NOT complain */ r = q+1; r = &xp->a; /* This should NOT complain */ r = &xp->b; r = &(*xp).a; r = &(*xp).b; r = &x.a; r = &x.b; r = py->a; r = py->a+1; r = &py->a[0]; } /* * check-name: noderef attribute * * check-error-start noderef.c:24:12: warning: incorrect type in assignment (different modifiers) noderef.c:24:12: expected char *[noderef] *q2 noderef.c:24:12: got char [noderef] **<noident> * check-error-end */
I noticed this behavior and was wondering if it is intended: bapiya. cat /tmp/q.c __attribute__((force)) int *p; int q = *p; bapiya. sparse -Wno-decl /tmp/q.c /tmp/q.c:2:10: warning: dereference of noderef expression If so it would be good to know all the rules for "force" -- I stumbled across this by accident but then was wondering if there are other rules I don't know of.
(In reply to Tom Tromey from comment #3) > I noticed this behavior and was wondering if it is intended: > > bapiya. cat /tmp/q.c > __attribute__((force)) int *p; > int q = *p; > bapiya. sparse -Wno-decl /tmp/q.c > /tmp/q.c:2:10: warning: dereference of noderef expression > > > If so it would be good to know all the rules for "force" -- > I stumbled across this by accident but then was wondering > if there are other rules I don't know of. What version of sparse did you try that with? I can't seem to reproduce that with the current version, nor with older versions.
(In reply to Josh Triplett from comment #4) > What version of sparse did you try that with? I can't seem to reproduce > that with the current version, nor with older versions. The one from Fedora 20: bapiya. sparse --version 0.4.4 bapiya. rpm -q sparse sparse-0.4.5.rc1-3.fc20.x86_64 FWIW I have an initial patch for the address_space and force attributes. noderef looks a little harder, as I think it will need to walk over expressions after they are built in order to handle the "&" case properly.
Null pointer constants are treated specially, which makes sense, but only if they have type "void *" and are in address space 0. That is, this works: #define NULL ((__attribute__ ((address_space (0))) void *) 0) __attribute__((address_space (1))) int *p = NULL; But this gets a warning: #define NULL ((__attribute__ ((address_space (1))) void *) 0) __attribute__((address_space (0))) int *p = NULL; And so does this: #define NULL ((int *) 0) __attribute__((address_space (1))) int *p = NULL; I'm not sure whether that last one ought to be an error or not.
(In reply to Tom Tromey from comment #6) > Null pointer constants are treated specially, which makes sense, > but only if they have type "void *" and are in address space 0. Otherwise, they're not a null pointer constant, so they're not treated specially. :) A null pointer constant must be in address space 0; that's for compatibility with the standard definition of NULL, to avoid needing unique NULL constants for each possible address space (USER_NULL, IOMEM_NULL). I don't think it makes sense to treat a 0 in address space 1 as a null pointer constant; safer to give a warning for mixing address spaces. The goal isn't to treat 0 magically as an address in all address spaces; it's specifically to treat NULL as the null for all address spaces. > That is, this works: > > #define NULL ((__attribute__ ((address_space (0))) void *) 0) > __attribute__((address_space (1))) int *p = NULL; > > But this gets a warning: > > #define NULL ((__attribute__ ((address_space (1))) void *) 0) > __attribute__((address_space (0))) int *p = NULL; I can't think of a legitimate reason to have a null pointer constant in a non-zero address space; there's already a null pointer constant, NULL, effectively in all address spaces, so why would you want to redefine it? And on the contrary, I can think of a very good reason to *have* this warning: suppose you wanted to define an INVALID_FOO_POINTER in the foo address space, and you decided to use 0 as the invalid value. You should get a warning if you try to use INVALID_FOO_POINTER with a non-foo pointer type; it shouldn't magically pass silently just because the chosen value is 0. > And so does this: > > #define NULL ((int *) 0) > __attribute__((address_space (1))) int *p = NULL; > > > I'm not sure whether that last one ought to be an error or not. That isn't a null pointer constant, since it isn't (void *); it can't be converted to any other pointer type without warning, and I don't think it's unreasonable to say it can't be converted to any other address space without warning either.
Arguably the *right* way to solve that would be to support __null for C as well as for C++.
(In reply to Josh Triplett from comment #7) > I can't think of a legitimate reason to have a null pointer constant in a > non-zero address space; there's already a null pointer constant, NULL, > effectively in all address spaces, so why would you want to redefine it? > That isn't a null pointer constant, since it isn't (void *); it can't be > converted to any other pointer type without warning, and I don't think it's > unreasonable to say it can't be converted to any other address space without > warning either. Thanks. While the one case did seem borderline to me, overall my concern is really about trying to understand all the cases, so I can document the feature nicely.
Relatedly, could you say what the option "-Wcast-to-as" provides beyond the normal warnings about changing address spaces? I wonder if this is something I should be pulling in as well. "man sparse" doesn't really say much here.
(In reply to Tom Tromey from comment #10) > Relatedly, could you say what the option "-Wcast-to-as" provides > beyond the normal warnings about changing address spaces? > I wonder if this is something I should be pulling in as well. > "man sparse" doesn't really say much here. /tmp$ cat test.c static void *p; static __attribute__((address_space(1))) void *p2 = p; static __attribute__((address_space(1))) void *p3 = (__attribute__((address_space(1))) void *)p; static __attribute__((address_space(1))) void *p4 = (__attribute__((address_space(1),force)) void *)p; /tmp$ ~/src/sparse/sparse -Waddress-space test.c test.c:2:53: warning: incorrect type in initializer (different address spaces) test.c:2:53: expected void <asn:1>*static [toplevel] p2 test.c:2:53: got void *static [toplevel] p /tmp$ ~/src/sparse/sparse -Waddress-space -Wcast-to-as test.c test.c:2:53: warning: incorrect type in initializer (different address spaces) test.c:2:53: expected void <asn:1>*static [toplevel] p2 test.c:2:53: got void *static [toplevel] p test.c:3:54: warning: cast adds address space to expression (<asn:1>) Without -Wcast-to-as, you won't get a warning for unforced casts that add an address space. Personally, I'd actually suggest merging the two in GCC, and always issuing both sets of warnings. I'd also suggest including the warnings in GCC's -Wall, given that you'll only see them if you explicitly use an address_space attribute.
(In reply to Tom Tromey from comment #9) > (In reply to Josh Triplett from comment #7) > > > I can't think of a legitimate reason to have a null pointer constant in a > > non-zero address space; there's already a null pointer constant, NULL, > > effectively in all address spaces, so why would you want to redefine it? > > > That isn't a null pointer constant, since it isn't (void *); it can't be > > converted to any other pointer type without warning, and I don't think it's > > unreasonable to say it can't be converted to any other address space without > > warning either. > > Thanks. While the one case did seem borderline to me, overall my > concern is really about trying to understand all the cases, so I can > document the feature nicely. For the documentation, I'd suggest making it very clear that the magic "works in all addresss spaces" behavior applies only to NULL ((void *)0), not to any arbitrary pointer with the value 0.
(In reply to H. Peter Anvin from comment #8) > Arguably the *right* way to solve that would be to support __null for C as > well as for C++. __null or nullptr?
(In reply to Josh Triplett from comment #11) > Without -Wcast-to-as, you won't get a warning for unforced casts that add an > address space. Thanks! > Personally, I'd actually suggest merging the two in GCC, and always issuing > both sets of warnings. I'd also suggest including the warnings in GCC's > -Wall, given that you'll only see them if you explicitly use an > address_space attribute. My current patch adds just -Waddress-space and enables this warning by default -- similar to what was decided for -Wdesignated-init. This seems like something that might be discussed and changed during the patch submission though.
(In reply to Tom Tromey from comment #14) > (In reply to Josh Triplett from comment #11) > > Personally, I'd actually suggest merging the two in GCC, and always issuing > > both sets of warnings. I'd also suggest including the warnings in GCC's > > -Wall, given that you'll only see them if you explicitly use an > > address_space attribute. > > My current patch adds just -Waddress-space and enables this > warning by default -- similar to what was decided for -Wdesignated-init. > This seems like something that might be > discussed and changed during the patch submission though. Given that it only applies if you use the attribute, enabling it by default seems even better than putting it in -Wall, unless there's some fundamental objection to that.
Josh: nullptr pollutes the C user namespace, so it's not an option.
It seems that "force" works on function parameters and casts but not direct assignments: bapiya. nl -ba /tmp/q.c 1 #define A(N) __attribute__((address_space (N))) 2 #define force __attribute__((force)) 3 4 force int *ok; 5 A(1) int *nope; 6 7 void g(force int *p) 8 { 9 ok = p; 10 } 11 12 void f(void) 13 { 14 g(nope); 15 ok = nope; 16 } bapiya. ./sparse -Wno-decl -Waddress-space /tmp/q.c /tmp/q.c:15:6: warning: incorrect type in assignment (different address spaces) /tmp/q.c:15:6: expected int *[addressable] [toplevel] [assigned] ok /tmp/q.c:15:6: got int <asn:1>*[addressable] [toplevel] nope (This is using git master sparse from today)
(In reply to Tom Tromey from comment #17) > It seems that "force" works on function parameters and casts > but not direct assignments: It's also an error in conditional expressions and in a "return". I can implement this exactly but I'm curious whether it is intended.
(In reply to Tom Tromey from comment #18) > (In reply to Tom Tromey from comment #17) > > It seems that "force" works on function parameters and casts > > but not direct assignments: > > It's also an error in conditional expressions and in a "return". > > I can implement this exactly but I'm curious whether it is intended. I brought this exact case up on linux-sparse, and Christopher Li's (quite reasonable) perspective was that it doesn't really make sense to put "force" on a variable to begin with (as opposed to a function parameter). Given that, I think one of two behaviors would be reasonable: either prohibit force entirely on non-parameter variable declarations, or allow it and treat it much like parameters (ignore extended type differences on assignment). I'm mildly inclined towards the latter. I don't, however, think it's sensible to reproduce sparse's behavior entirely here, allowing it but not having it take effect. Either prohibit it or give it a sensible semantic, preferably the latter.
(In reply to Josh Triplett from comment #19) > I brought this exact case up on linux-sparse, and Christopher Li's (quite > reasonable) perspective was that it doesn't really make sense to put "force" > on a variable to begin with (as opposed to a function parameter). Given > that, I think one of two behaviors would be reasonable: either prohibit > force entirely on non-parameter variable declarations, or allow it and treat > it much like parameters (ignore extended type differences on assignment). > I'm mildly inclined towards the latter. > > I don't, however, think it's sensible to reproduce sparse's behavior > entirely here, allowing it but not having it take effect. Either prohibit > it or give it a sensible semantic, preferably the latter. Allowing it is definitely simpler to implement. Disallowing it for ordinary declarations would need some research on my part. BTW if you want to try it out I have a branch: https://github.com/tromey/gcc/tree/add-sparse-attributes
(In reply to Tom Tromey from comment #20) > BTW if you want to try it out I have a branch: > https://github.com/tromey/gcc/tree/add-sparse-attributes This still needs a bit of work. I rebased it to be more patchlike and wrote some ChangeLog entries, but I still need to verify that all the behavior is correct (I forgot a lot of state...) and also write the documentation. That said, recently I've been wondering whether this could be better done as a plugin. I started down the road of simply patching gcc due, I think, to the designated_init attribute, which can't really be done as a plugin. But it seems that perhaps noderef/force/address_space could be; and perhaps also bitwise and nocast. In the "pro" column, as a plugin it could be maintained elsewhere. That might be interesting. In the "con" column, it's a pain if multiple projects want to use these checks. Then it's just one more thing to fetch. I'm curious to hear your thoughts on the subject.
(In reply to Tom Tromey from comment #21) > In the "pro" column, as a plugin it could be maintained elsewhere. > That might be interesting. > > In the "con" column, it's a pain if multiple projects want to > use these checks. Then it's just one more thing to fetch. * We could add plugins to the GCC repository for things that are considered generally useful but we don't want to bloat standard gcc. I am sure the FSF will be happier if plugins live in the GCC repository and they are assigned to them than if not. * A plugin living in the GCC repository will likely have a lower barrier for acceptance than code added to GCC.
some data points based on my experience with the 'checker' gcc plugin in PaX: 1. the C address space infrastructure available since gcc 4.6 can be sort of coerced into implementing the __user/__kernel/etc address spaces and it works reasonably well (i'd say even better than sparse as it produces no false positives in my experience and caught real bugs such as CVE-2014-0038). 2. __force itself presents a problem as its semantics isn't well defined and only sparse knows how to model it. in gcc it cannot be an attribute as attributes apply to the outermost variable/etc, e.g., you can't use them on a pointee in a pointer context. what i did instead is that i introduced new address spaces (__force_user/__force_kernel so far, __rcu/__iomem/etc will need more of these) that replace the '__force something' combination with __force_something (yes, this needs patching on the kernel side, and i haven't done a thorough job of it but it works on my smaller configs at least). this way the hijacked targetm.addr_space.legitimate_address_p callback can be taught to allow/disallow the intended conversions. 3. designated_init is a tricky problem because by the time a plugin can examine variable initializers, gcc will have lost the information. however with a trick such unwanted initializers can instead be turned into a compile error (that existing gcc infrastructure can detect). you can find it in spender's randomize_layout plugin that's distributed in grsecurity. 4. as for maintaining a plugin for kernel and/or other use: inside the kernel it'll need some kbuild infrastructure (there's one in PaX already, though it's probably not 100% complete) and it's worked fine for our users for the past 3+ years now. for more general use distros can package up plugins as they'd do with any library (as plugins are really nothing more than that). note also that keeping a plugin in the kernel tree will raise license problems (gplv2 vs gplv3) but i guess the kernel list is the better forum for discussing that.
(In reply to PaX Team from comment #23) > 3. designated_init is a tricky problem because by the time a plugin can > examine variable initializers, gcc will have lost the information. however > with a trick such unwanted initializers can instead be turned into a compile > error (that existing gcc infrastructure can detect). you can find it in > spender's randomize_layout plugin that's distributed in grsecurity. There is a patch for GCC that was basically approved in January: https://gcc.gnu.org/ml/gcc-patches/2014-01/msg01284.html I am not sure why it hasn't been committed yet.
(In reply to Manuel López-Ibáñez from comment #24) > There is a patch for GCC that was basically approved in January: > https://gcc.gnu.org/ml/gcc-patches/2014-01/msg01284.html > > I am not sure why it hasn't been committed yet. I'm going to submit the fixed up version this week.
(In reply to PaX Team from comment #23) > some data points based on my experience with the 'checker' gcc plugin in PaX: Hi. Thanks for your reply. I didn't easily find a git repository holding the checker plugin source. Is it available somewhere? > 1. the C address space infrastructure available since gcc 4.6 can be sort of > coerced into implementing the __user/__kernel/etc address spaces and it > works reasonably well (i'd say even better than sparse as it produces no > false positives in my experience and caught real bugs such as CVE-2014-0038). FWIW I looked into the existing C address space stuff in gcc and after some deliberation decided not to use it. It wasn't directly applicable and tricks like taking some subset of the address space values for use by the attribute would have meant difficult-to-test patches to various back ends. Your code apparently hijacks the target hook, which seems pretty clever, though I guess more suitable in a plugin than in gcc proper. > 2. __force itself presents a problem as its semantics isn't well defined and > only sparse knows how to model it. in gcc it cannot be an attribute as > attributes apply to the outermost variable/etc, e.g., you can't use them on > a pointee in a pointer context. Could you elaborate on this? I think I looked at all the sparse test cases here and I don't recall encountering any real issues (for address space I had to have a hack to deal with function return types, but this didn't seem to affect force). If you have extra tests not in sparse, that would be super.
(In reply to Tom Tromey from comment #21) > (In reply to Tom Tromey from comment #20) > > > BTW if you want to try it out I have a branch: > > https://github.com/tromey/gcc/tree/add-sparse-attributes > > This still needs a bit of work. > I rebased it to be more patchlike and wrote some ChangeLog > entries, but I still need to verify that all the behavior > is correct (I forgot a lot of state...) and also write the > documentation. > > That said, recently I've been wondering whether this could > be better done as a plugin. I started down the road of > simply patching gcc due, I think, to the designated_init > attribute, which can't really be done as a plugin. > But it seems that perhaps noderef/force/address_space could > be; and perhaps also bitwise and nocast. > > In the "pro" column, as a plugin it could be maintained elsewhere. > That might be interesting. > > In the "con" column, it's a pain if multiple projects want to > use these checks. Then it's just one more thing to fetch. > > I'm curious to hear your thoughts on the subject. Given the goal of providing compatibility with the existing support in Sparse (and potentially surpassing it in the future with the benefit of better analysis and compiler infrastructure backing it up), I strongly feel that this needs to exist in the default distribution of GCC, invokable without any barriers beyond an additional warning flag. (And usually not even that; anything that only emits warnings on code using an extension attribute should get turned on by default.) A plugin shipped with GCC and enabled by default could potentially provide that benefit as well, but that goes against the only "pro" you listed. I don't think maintaining this elsewhere makes sense. Meanwhile, the "con" you listed seems far more serious. I'd like to see all projects currently using Sparse able to transparently take advantage of this. Please let me know what I can do to help complete this branch. I'd be happy to help write the documentation, for instance.
> Please let me know what I can do to help complete this branch. I'd be happy > to help write the documentation, for instance. It's maybe not quite ready for testing. But if you want to give it a try and see how it fares, that would help. My spare time is pretty limited, so any sort of help is appreciated ... docs, fixes, checking that I got the test cases correct, taking over the branch entirely... :) FWIW I re-sent the designated_init patch for review. If you want to comment on that one that would also be great.
(In reply to Tom Tromey from comment #26) > > 2. __force itself presents a problem as its semantics isn't well defined and > > only sparse knows how to model it. in gcc it cannot be an attribute as > > attributes apply to the outermost variable/etc, e.g., you can't use them on > > a pointee in a pointer context. > > Could you elaborate on this? > I think I looked at all the sparse test cases here and I don't recall > encountering any real issues (for address space I had to have a hack to > deal with function return types, but this didn't seem to affect force). > If you have extra tests not in sparse, that would be super. If _Pragma can appear in the same places as __force, I think that would be a superior approach. The trivial approach of using _Pragma("GCC diagnostic ignore") does not seem to work in the middle of expressions, but that may be a defect of the current implementation (PR60875 and PR52116). Otherwise, a (type-generic?) __builtin_force_cast() perhaps would be even better. It seems strange and convoluted to use an attribute for this.
Please don't invent your own syntax for this. Use the one that has been widely deployed for years. Thanks.
(In reply to Tom Tromey from comment #28) > > Please let me know what I can do to help complete this branch. I'd be happy > > to help write the documentation, for instance. > > It's maybe not quite ready for testing. I did a bit more work on it now. Josh, could you take a look? It's git@github.com:tromey/gcc.git add-sparse-attributes Most useful, I think, would be a review of the tests, to make sure that they are complete and correct; and a review of the docs. force, adddress_space, and noderef are the final 5 commits on the branch. I didn't squash it all into submission form yet.
(In reply to Tom Tromey from comment #31) > force, adddress_space, and noderef are the final 5 commits on the branch. Err, 8 commits.
My current patch fails on some of the sparse validation tests. E.g., attr-in-parameter.c: attr_in_parameter.c:8:4: warning: assignment from pointer to different address space [-Waddress-space] This happens due to this line: static int (A *p); I have yet to debug this one. Also from noderef.c I found out that sparse accepts "noderef" on non-pointer types: struct x __A x; I hadn't realized this was possible. It seems somewhat weird to me, but I suppose I'll update my patch to follow. This also affects address_space, as shown by typeof-attribute.c: unsigned int __percpu x;
Created attachment 33277 [details] initial patch This is my current patch. I'm uploading it for safekeeping as I probably won't be working on this in the near future. Note that as mentioned in the comments, this doesn't pass the sparse validation suite. So, it needs some work... feel free to take it over if you are so moved. A good chunk of it should still be relevant even with the needed updates.
Created attachment 53631 [details] Refreshed version of Tom Tromey's patch This is a refreshed version of Tom Tromey's patch from 2014, against last week's trunk (specifically, against dc829c7613ddf562d1aecaf22eda965e87108ac8). The new tests pass; am currently attempting a bootstrap and full regression test. Has the same caveats that Tom noted in the earlier comments.
(In reply to H. Peter Anvin from comment #16) > Josh: nullptr pollutes the C user namespace, so it's not an option. FWIW, C23 added the nullptr keyword which gcc already implements: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=60d84e82639e25b025a926b19ec5a92fba4447ce
One would assume that there would be __foo__ aliases for the attribute names like all the other ones.