| Message ID | 20210120173612.20913-9-will@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Create 'old' ptes for faultaround mappings on arm64 with hardware access flag | expand |
On Wed, Jan 20, 2021 at 9:36 AM Will Deacon <will@kernel.org> wrote: > > The fields of this struct are only ever read after being initialised, so > mark it 'const' before somebody tries to modify it again. > > Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Will Deacon <will@kernel.org> > --- > include/linux/mm.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index e0f056753bef..7ff3d9817d38 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -514,7 +514,7 @@ static inline bool fault_flag_allow_retry_first(unsigned int flags) > * pgoff should be used in favour of virtual_address, if possible. > */ > struct vm_fault { > - struct { > + const struct { > struct vm_area_struct *vma; /* Target VMA */ > gfp_t gfp_mask; /* gfp mask to be used for allocations */ > pgoff_t pgoff; /* Logical page offset based on vma */ Is there a difference between: + const struct { + struct vm_area_struct *vma; /* Target VMA */ + gfp_t gfp_mask; /* gfp mask to be used for allocations */ + pgoff_t pgoff; /* Logical page offset based on vma */ + unsigned long address; /* Faulting virtual address */ + }; vs + struct { + struct vm_area_struct * const vma; /* Target VMA */ + const gfp_t gfp_mask; /* gfp mask to be used for allocations */ + const pgoff_t pgoff; /* Logical page offset based on vma */ + const unsigned long address; /* Faulting virtual address */ + }; ie. marking the members const vs the anonymous struct? Does anything need to change in the use of these structures? > -- > 2.30.0.284.gd98b1dd5eaa7-goog >
On Wed, Jan 20, 2021 at 10:27 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > Is there a difference between: [ const unnamed struct and individual const members ] Semantically? No. Syntactically the "group the const members together" is a lot cleaner, imho. Not just from a "just a single const" standpoint, but from a "code as documentation" standpoint. But I guess to avoid the clang issue, we could do the "mark individual fields" thing. (It turns out that sparse gets this wrong too, so it's not just clang). Linus
On Wed, Jan 20, 2021 at 11:02:06AM -0800, Linus Torvalds wrote: > On Wed, Jan 20, 2021 at 10:27 AM Nick Desaulniers > <ndesaulniers@google.com> wrote: > > > > Is there a difference between: [ const unnamed struct and individual const members ] > > Semantically? No. > > Syntactically the "group the const members together" is a lot cleaner, > imho. Not just from a "just a single const" standpoint, but from a > "code as documentation" standpoint. > > But I guess to avoid the clang issue, we could do the "mark individual > fields" thing. I'd prefer to wait until the bug against LLVM has been resolved before we try to work around anything. Although I couldn't find any other examples like this in the kernel, requiring all of the member fields to be marked as 'const' still feels pretty fragile to me; it's only a matter of time before new non-const fields get added, at which point the temptation for developers to remove 'const' from other fields when it gets in the way is pretty high. None of this is bullet-proof, of course, but if clang ends up emitting a warning (even if it's gated behind an option) then I think we're in a good place. > (It turns out that sparse gets this wrong too, so it's not just clang). Adding Luc, as hopefully that's fixable. Will
On Thu, Jan 21, 2021 at 5:11 AM Will Deacon <will@kernel.org> wrote: > > On Wed, Jan 20, 2021 at 11:02:06AM -0800, Linus Torvalds wrote: > > On Wed, Jan 20, 2021 at 10:27 AM Nick Desaulniers > > <ndesaulniers@google.com> wrote: > > > > > > Is there a difference between: [ const unnamed struct and individual const members ] > > > > Semantically? No. > > > > Syntactically the "group the const members together" is a lot cleaner, > > imho. Not just from a "just a single const" standpoint, but from a > > "code as documentation" standpoint. > > > > But I guess to avoid the clang issue, we could do the "mark individual > > fields" thing. > > I'd prefer to wait until the bug against LLVM has been resolved before we > try to work around anything. Although I couldn't find any other examples > like this in the kernel, requiring all of the member fields to be marked as > 'const' still feels pretty fragile to me; it's only a matter of time before > new non-const fields get added, at which point the temptation for developers > to remove 'const' from other fields when it gets in the way is pretty high. What's to stop a new non-const field from getting added outside the const qualified anonymous struct? What's to stop someone from removing const from the anonymous struct? What's to stop a number of callers from manipulating the structure temporarily before restoring it when returning by casting away the const? Code review. Using a non-toolchain-portable solution certainly could be considered more fragile. It's always possible that the resolution is the C standards body goes the C++ route, at which point GCC would be forced to address this and potentially change behavior. Kind of like how people avoid going to court since things are never guaranteed to work out in their favor. > > None of this is bullet-proof, of course, but if clang ends up emitting a > warning (even if it's gated behind an option) then I think we're in a good > place. > > > (It turns out that sparse gets this wrong too, so it's not just clang). > > Adding Luc, as hopefully that's fixable. > > Will -- Thanks, ~Nick Desaulniers
On Thu, Jan 21, 2021 at 11:24:36AM -0800, Nick Desaulniers wrote: > On Thu, Jan 21, 2021 at 5:11 AM Will Deacon <will@kernel.org> wrote: > > > > On Wed, Jan 20, 2021 at 11:02:06AM -0800, Linus Torvalds wrote: > > > On Wed, Jan 20, 2021 at 10:27 AM Nick Desaulniers > > > <ndesaulniers@google.com> wrote: > > > > > > > > Is there a difference between: [ const unnamed struct and individual const members ] > > > > > > Semantically? No. > > > > > > Syntactically the "group the const members together" is a lot cleaner, > > > imho. Not just from a "just a single const" standpoint, but from a > > > "code as documentation" standpoint. > > > > > > But I guess to avoid the clang issue, we could do the "mark individual > > > fields" thing. > > > > I'd prefer to wait until the bug against LLVM has been resolved before we > > try to work around anything. Although I couldn't find any other examples > > like this in the kernel, requiring all of the member fields to be marked as > > 'const' still feels pretty fragile to me; it's only a matter of time before > > new non-const fields get added, at which point the temptation for developers > > to remove 'const' from other fields when it gets in the way is pretty high. > > What's to stop a new non-const field from getting added outside the > const qualified anonymous struct? > What's to stop someone from removing const from the anonymous struct? > What's to stop a number of callers from manipulating the structure > temporarily before restoring it when returning by casting away the > const? > > Code review. Sure, but here we are cleaning up this stuff, so I think review only gets you so far. To me: const struct { int foo; long bar; }; clearly says "don't modify fields of this struct", whereas: struct { const int foo; const long bar; }; says "don't modify foo or bar, but add whatever you like on the end" and that's the slippery slope. So then we end up with the eye-sore of: const struct { const int foo; const long bar; }; and maybe that's the right answer, but I'm just saying we should wait for clang to make up its mind first. It's not like this is a functional problem, and there are enough GCC users around that we're not exactly in a hurry. Will
On Thu, Jan 21, 2021 at 5:11 AM Will Deacon <will@kernel.org> wrote: > > > (It turns out that sparse gets this wrong too, so it's not just clang). > > Adding Luc, as hopefully that's fixable. I had talked to Luc about this earlier, and he just sent out a fix for sparse. It's not in the git repo yet, but it's coming. Having looked at what sparse does, I suspect the clang behavior stems from a similar approach to looking up unnamed struct/union members. And the sparse fix was fairly straightforward: changing the _lookup_ is painful, because that's late and trying to fix types after-the-fact is just not great. But just (recursively) modifying the type modifiers at type parsing time for anonymous struct/union members is quite straightforward, probably in clang too. So honestly - I think the clang push-back was by somebody who thought it would be nasty to fix, and who was thus actively trying to mis-read the standards language. I'm not willing to argue with compiler people who do standards language weasel-wording (I've seen it before, my life is too short to deal with those kinds of people), but maybe Nick is more used to deal with clang people. Nick - I suspect that the sparse type system model is different enough from the clang one that the sparse patch is not really helpful as a "look, this is how it was done in sparse, maybe clang can do something similar", but I'll bounce it to you separately anyway just in case. Maybe your clang knowledge means that you go "yeah, in clang that function is called 'xyz', and we can do something very similar". Linus
On Thu, Jan 21, 2021 at 1:28 PM Will Deacon <will@kernel.org> wrote: > > On Thu, Jan 21, 2021 at 11:24:36AM -0800, Nick Desaulniers wrote: > > On Thu, Jan 21, 2021 at 5:11 AM Will Deacon <will@kernel.org> wrote: > > > > > > On Wed, Jan 20, 2021 at 11:02:06AM -0800, Linus Torvalds wrote: > > > > On Wed, Jan 20, 2021 at 10:27 AM Nick Desaulniers > > > > <ndesaulniers@google.com> wrote: > > > > > > > > > > Is there a difference between: [ const unnamed struct and individual const members ] > > > > > > > > Semantically? No. > > > > > > > > Syntactically the "group the const members together" is a lot cleaner, > > > > imho. Not just from a "just a single const" standpoint, but from a > > > > "code as documentation" standpoint. > > > > > > > > But I guess to avoid the clang issue, we could do the "mark individual > > > > fields" thing. > > > > > > I'd prefer to wait until the bug against LLVM has been resolved before we > > > try to work around anything. Although I couldn't find any other examples > > > like this in the kernel, requiring all of the member fields to be marked as > > > 'const' still feels pretty fragile to me; it's only a matter of time before > > > new non-const fields get added, at which point the temptation for developers > > > to remove 'const' from other fields when it gets in the way is pretty high. > > > > What's to stop a new non-const field from getting added outside the > > const qualified anonymous struct? > > What's to stop someone from removing const from the anonymous struct? > > What's to stop a number of callers from manipulating the structure > > temporarily before restoring it when returning by casting away the > > const? > > > > Code review. > > Sure, but here we are cleaning up this stuff, so I think review only gets > you so far. To me: > > const struct { > int foo; > long bar; > }; > > clearly says "don't modify fields of this struct", whereas: > > struct { > const int foo; > const long bar; > }; > > says "don't modify foo or bar, but add whatever you like on the end" and > that's the slippery slope. "but you could add additional non-const members on the end" for sure. Though going back to >> What's to stop a new non-const field from getting added outside the > > const qualified anonymous struct? my point with that is that the const anonymous struct is within a non-const anonymous struct. struct vm_fault { const { struct vm_area_struct *vma; gfp_t gfp_mask; pgoff_t pgoff; unsigned long address; // Your point is about new member additions here, IIUC }; // My point: what's to stop someone from adding a new non-const member here? unsigned int flags; pmd_t *pmd; pud_t *pud; ... // or here? }; The const anonymous struct will help prevent additions of non-const members to the anonymous struct, sure; but if someone really wanted a new non-const member in a `struct vm_fault` instance they're just going to go around the const anonymous struct. Or is there something more I'm missing about the order of the members of struct vm_fault? > So then we end up with the eye-sore of: > > const struct { > const int foo; > const long bar; > }; > > and maybe that's the right answer, but I'm just saying we should wait for > clang to make up its mind first. It's not like this is a functional problem, > and there are enough GCC users around that we're not exactly in a hurry. Yeah, I mean I'm happy to whip something up for Clang, even though I suspect it will get shot down in code review until there's more guidance from standards bodies. It doesn't hurt to try, and at least have a patch "waiting in the wings" should we hear back from WG14 that favors GCC's behavior. Who knows, maybe the guidance will be that WG21 should revisit this behavior for C++ to avoid divergence with C (as g++ and gcc currently do).
Hey Nick, On Fri, Jan 22, 2021 at 11:10:50AM -0800, Nick Desaulniers wrote: > On Thu, Jan 21, 2021 at 1:28 PM Will Deacon <will@kernel.org> wrote: > > On Thu, Jan 21, 2021 at 11:24:36AM -0800, Nick Desaulniers wrote: > > > On Thu, Jan 21, 2021 at 5:11 AM Will Deacon <will@kernel.org> wrote: > > Sure, but here we are cleaning up this stuff, so I think review only gets > > you so far. To me: > > > > const struct { > > int foo; > > long bar; > > }; > > > > clearly says "don't modify fields of this struct", whereas: > > > > struct { > > const int foo; > > const long bar; > > }; > > > > says "don't modify foo or bar, but add whatever you like on the end" and > > that's the slippery slope. > > "but you could add additional non-const members on the end" for sure. > Though going back to > > >> What's to stop a new non-const field from getting added outside the > > > const qualified anonymous struct? > > my point with that is that the const anonymous struct is within a > non-const anonymous struct. > > struct vm_fault { > const { > struct vm_area_struct *vma; > gfp_t gfp_mask; > pgoff_t pgoff; > unsigned long address; > // Your point is about new member additions here, IIUC > }; > // My point: what's to stop someone from adding a new non-const member here? > unsigned int flags; > pmd_t *pmd; > pud_t *pud; > ... > // or here? > }; > > The const anonymous struct will help prevent additions of non-const > members to the anonymous struct, sure; but if someone really wanted a > new non-const member in a `struct vm_fault` instance they're just > going to go around the const anonymous struct. Or is there something > more I'm missing about the order of the members of struct vm_fault? All I was trying to say is that, given a struct with a mixture of const and non-const members, I would be less hesitant to remove 'const' from one of the members if it helped me get something else done. Having the 'const' on the struct itself, however, would deter me because at that point its clear that you're not supposed to be modifying this stuff. That's the difference between the "Your point" and "My point" lines above. But honestly, I can't say I particularly enjoy arguing about these idiosyncracies; I'd just rather wait for the dust to settle with clang before we add code to deal with that outcome. > > So then we end up with the eye-sore of: > > > > const struct { > > const int foo; > > const long bar; > > }; > > > > and maybe that's the right answer, but I'm just saying we should wait for > > clang to make up its mind first. It's not like this is a functional problem, > > and there are enough GCC users around that we're not exactly in a hurry. > > Yeah, I mean I'm happy to whip something up for Clang, even though I > suspect it will get shot down in code review until there's more > guidance from standards bodies. It doesn't hurt to try, and at least > have a patch "waiting in the wings" should we hear back from WG14 that > favors GCC's behavior. Who knows, maybe the guidance will be that > WG21 should revisit this behavior for C++ to avoid divergence with C > (as g++ and gcc currently do). I mean, a warning doesn't seem controversial to me, especially as opinions certainly seem to be divided about what the right behaviour should be here. Will
diff --git a/include/linux/mm.h b/include/linux/mm.h index e0f056753bef..7ff3d9817d38 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -514,7 +514,7 @@ static inline bool fault_flag_allow_retry_first(unsigned int flags) * pgoff should be used in favour of virtual_address, if possible. */ struct vm_fault { - struct { + const struct { struct vm_area_struct *vma; /* Target VMA */ gfp_t gfp_mask; /* gfp mask to be used for allocations */ pgoff_t pgoff; /* Logical page offset based on vma */
The fields of this struct are only ever read after being initialised, so mark it 'const' before somebody tries to modify it again. Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Will Deacon <will@kernel.org> --- include/linux/mm.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)