| Message ID | 20190729211014.39333-1-ndesaulniers@google.com |
|---|---|
| State | Accepted |
| Commit | 12051b318bc3ce5b42d6d786191008284b067d83 |
| Headers | show |
| Series | mips: avoid explicit UB in assignment of mips_io_port_base | expand |
On Mon, Jul 29, 2019 at 02:10:12PM -0700, Nick Desaulniers wrote: > The code in question is modifying a variable declared const through > pointer manipulation. Such code is explicitly undefined behavior, and > is the lone issue preventing malta_defconfig from booting when built > with Clang: > > If an attempt is made to modify an object defined with a const-qualified > type through use of an lvalue with non-const-qualified type, the > behavior is undefined. > > LLVM is removing such assignments. A simple fix is to not declare > variables const that you plan on modifying. Limiting the scope would be > a better method of preventing unwanted writes to such a variable. > > Further, the code in question mentions "compiler bugs" without any links > to bug reports, so it is difficult to know if the issue is resolved in > GCC. The patch was authored in 2006, which would have been GCC 4.0.3 or > 4.1.1. The minimal supported version of GCC in the Linux kernel is > currently 4.6. > > For what its worth, there was UB before the commit in question, it just > added a barrier and got lucky IRT codegen. I don't think there's any > actual compiler bugs related, just runtime bugs due to UB. > > Link: https://github.com/ClangBuiltLinux/linux/issues/610 > Fixes: 966f4406d903 ("[MIPS] Work around bad code generation for <asm/io.h>.") > Reported-by: Nathan Chancellor <natechancellor@gmail.com> > Debugged-by: Nathan Chancellor <natechancellor@gmail.com> > Suggested-by: Eli Friedman <efriedma@quicinc.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> Tested-by: Nathan Chancellor <natechancellor@gmail.com>
On Mon, 29 Jul 2019, Nick Desaulniers wrote: > The code in question is modifying a variable declared const through > pointer manipulation. Such code is explicitly undefined behavior, and > is the lone issue preventing malta_defconfig from booting when built > with Clang: > > If an attempt is made to modify an object defined with a const-qualified > type through use of an lvalue with non-const-qualified type, the > behavior is undefined. > > LLVM is removing such assignments. A simple fix is to not declare > variables const that you plan on modifying. Limiting the scope would be > a better method of preventing unwanted writes to such a variable. > > Further, the code in question mentions "compiler bugs" without any links > to bug reports, so it is difficult to know if the issue is resolved in > GCC. The patch was authored in 2006, which would have been GCC 4.0.3 or > 4.1.1. The minimal supported version of GCC in the Linux kernel is > currently 4.6. It's somewhat older than that. My investigation points to: commit c94e57dcd61d661749d53ee876ab265883b0a103 Author: Ralf Baechle <ralf@linux-mips.org> Date: Sun Nov 25 09:25:53 2001 +0000 Cleanup of include/asm-mips/io.h. Now looks neat and harmless. However the purpose of the arrangement does not appear to me to be particularly specific to a compiler version. > For what its worth, there was UB before the commit in question, it just > added a barrier and got lucky IRT codegen. I don't think there's any > actual compiler bugs related, just runtime bugs due to UB. Does your solution preserves the original purpose of the hack though as documented in the comment you propose to be removed? Clearly it was defined enough to work for almost 18 years, so it would be good to keep the optimisation functionally by using different means that do not rely on UB. This variable is assigned at most once throughout the life of the kernel and then early on, so considering it r/w with all the consequences for all accesses does not appear to me to be a good use of it. Maybe a piece of inline asm to hide the initialisation or suchlike then? Maciej
Hello, On Mon, Jul 29, 2019 at 11:16:45PM +0100, Maciej W. Rozycki wrote: > On Mon, 29 Jul 2019, Nick Desaulniers wrote: > > The code in question is modifying a variable declared const through > > pointer manipulation. Such code is explicitly undefined behavior, and > > is the lone issue preventing malta_defconfig from booting when built > > with Clang: > > > > If an attempt is made to modify an object defined with a const-qualified > > type through use of an lvalue with non-const-qualified type, the > > behavior is undefined. > > > > LLVM is removing such assignments. A simple fix is to not declare > > variables const that you plan on modifying. Limiting the scope would be > > a better method of preventing unwanted writes to such a variable. > > > > Further, the code in question mentions "compiler bugs" without any links > > to bug reports, so it is difficult to know if the issue is resolved in > > GCC. The patch was authored in 2006, which would have been GCC 4.0.3 or > > 4.1.1. The minimal supported version of GCC in the Linux kernel is > > currently 4.6. > > It's somewhat older than that. My investigation points to: > > commit c94e57dcd61d661749d53ee876ab265883b0a103 > Author: Ralf Baechle <ralf@linux-mips.org> > Date: Sun Nov 25 09:25:53 2001 +0000 > > Cleanup of include/asm-mips/io.h. Now looks neat and harmless. > > However the purpose of the arrangement does not appear to me to be > particularly specific to a compiler version. Agreed - I don't think the code here talks about compiler bugs at all, it talks about emitting extra unnecessary loads & says there's a codegen "issue" which I interpret in this context to simply mean that the generated code is suboptimal. See also this previous patch which aimed to remove the const too, though for other reasons; namely LTO: https://lore.kernel.org/linux-mips/20180616154745.28230-1-hauke@hauke-m.de/T/#u As I measured there this does indeed have an impact on code size, though it's not infeasibly large or anything. > > For what its worth, there was UB before the commit in question, it just > > added a barrier and got lucky IRT codegen. I don't think there's any > > actual compiler bugs related, just runtime bugs due to UB. > > Does your solution preserves the original purpose of the hack though as > documented in the comment you propose to be removed? > > Clearly it was defined enough to work for almost 18 years, so it would be > good to keep the optimisation functionally by using different means that > do not rely on UB. This variable is assigned at most once throughout the > life of the kernel and then early on, so considering it r/w with all the > consequences for all accesses does not appear to me to be a good use of > it. > > Maybe a piece of inline asm to hide the initialisation or suchlike then? That could work as a replacement hack. As I mentioned in the thread linked above a less hacky, though more extensive & invasive change might be to move our I/O area to a fixmap which ought to produce even better code since the addresses would become compile-time constant. I'd settle for either approach for now though. Thanks, Paul
Sorry for the delayed response, literally sent the patch then went on vacation. On Mon, Jul 29, 2019 at 3:16 PM Maciej W. Rozycki <macro@linux-mips.org> wrote: > > On Mon, 29 Jul 2019, Nick Desaulniers wrote: > > > The code in question is modifying a variable declared const through > > pointer manipulation. Such code is explicitly undefined behavior, and > > is the lone issue preventing malta_defconfig from booting when built > > with Clang: > > > > If an attempt is made to modify an object defined with a const-qualified > > type through use of an lvalue with non-const-qualified type, the > > behavior is undefined. > > > > LLVM is removing such assignments. A simple fix is to not declare > > variables const that you plan on modifying. Limiting the scope would be > > a better method of preventing unwanted writes to such a variable. This is now documented in the LLVM release notes for Clang-9: https://github.com/llvm/llvm-project/commit/e39e79358fcdd5d8ad809defaa821f0bbfa809a5 > > > > Further, the code in question mentions "compiler bugs" without any links > > to bug reports, so it is difficult to know if the issue is resolved in > > GCC. The patch was authored in 2006, which would have been GCC 4.0.3 or > > 4.1.1. The minimal supported version of GCC in the Linux kernel is > > currently 4.6. > > It's somewhat older than that. My investigation points to: > > commit c94e57dcd61d661749d53ee876ab265883b0a103 > Author: Ralf Baechle <ralf@linux-mips.org> > Date: Sun Nov 25 09:25:53 2001 +0000 > > Cleanup of include/asm-mips/io.h. Now looks neat and harmless. Oh indeed, great find! So it looks to me like the order of events is: 1. https://github.com/jaaron/linux-mips-ip30/commit/c94e57dcd61d661749d53ee876ab265883b0a103 in 2001 first introduces the UB. mips_io_port_base is defined non-const in arch/mips/kernel/setup.c, but then declared extern const (and modified via UB) in include/asm-mips/io.h. A setter is created, but not a getter (I'll revisit this below). This appears to work (due to luck) for a few years until: 2. https://github.com/mpe/linux-fullhistory/commit/966f4406d903a4214fdc74bec54710c6232a95b8 in 2006 adds a compiler barrier (reload all variables) and this appears to work. The commit message mentions that reads after modification of the const variable were buggy (likely GCC started taking advantage of the explicit UB around this time as well). This isn't a fix for UB (more thoughts below), but appears to work. 3. https://github.com/llvm/llvm-project/commit/b45631090220b732e614b5530bbd1d230eb9d38e in 2019 removes writes to const variables in LLVM as that's explicit UB. We observe the boot failure in mips and narrow it down to this instance. I can see how throwing a compiler barrier in there made subsequent reads after UB writes appear to work, but that was more due to luck and implementation details of GCC than the heart of the issue (ie. not writing code that is explicitly undefined behavior)(and could change in future versions of GCC). Stated another way, the fix for explicit UB is not hacks, but avoiding the UB by rewriting the problematic code. > However the purpose of the arrangement does not appear to me to be > particularly specific to a compiler version. > > > For what its worth, there was UB before the commit in question, it just > > added a barrier and got lucky IRT codegen. I don't think there's any > > actual compiler bugs related, just runtime bugs due to UB. > > Does your solution preserves the original purpose of the hack though as > documented in the comment you propose to be removed? The function modified simply writes to a global variable. It's not clear to my why the value about to be modified would EVER be loaded before modification. > Clearly it was defined enough to work for almost 18 years, so it would be > good to keep the optimisation functionally by using different means that > do not rely on UB. "Defined enough" ??? https://youtu.be/Aq_1l316ow8?t=17 > This variable is assigned at most once throughout the > life of the kernel and then early on, so considering it r/w with all the > consequences for all accesses does not appear to me to be a good use of > it. Note: it's not possible to express the semantics of a "write once variable" in C short of static initialization (AFAIK, without explicit violation of UB, but Cunningham's Law may apply). (set_io_port_base is called in ~20 places) Thinking more about this while I was away, I think what this code has needed since 2001 is proper encapsulation. If you want a variable that is written from one place only, but readable throughout, then the pattern I'd use is: 1. declare a getter in a .h file. 2. define/qualify `mips_io_port_base` as `static` and non-const in a .c file where it's modified. 3. define the getter and setter in the above .c file. That would rely on linkage to limit the visibility of the symbol for modification. But, we'd then need to export the getter, vs the symbol itself. There's also on the order of ~20 call sites that would need to be changed to invoke the getter rather than read the raw variable. Also, it's unlikely the getter gets inlined across translation units (short of LTO, which the mainline kernel doesn't support today). I think my patch here (https://lkml.org/lkml/2019/7/29/1636) is minimally and much less invasive. > Maybe a piece of inline asm to hide the initialisation or suchlike then? I think that would still be UB as the definition would not be changed; you'd still be modifying a variable declared const. -- Thanks, ~Nick Desaulniers
Hi Paul, Bumping this thread; we'd really like to be able to boot test another ISA in our CI. This lone patch is affecting our ability to boot. Can you please pick it up? https://lore.kernel.org/lkml/20190729211014.39333-1-ndesaulniers@google.com/ On Wed, Aug 7, 2019 at 2:12 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > Sorry for the delayed response, literally sent the patch then went on vacation. > > On Mon, Jul 29, 2019 at 3:16 PM Maciej W. Rozycki <macro@linux-mips.org> wrote: > > > > On Mon, 29 Jul 2019, Nick Desaulniers wrote: > > > > > The code in question is modifying a variable declared const through > > > pointer manipulation. Such code is explicitly undefined behavior, and > > > is the lone issue preventing malta_defconfig from booting when built > > > with Clang: > > > > > > If an attempt is made to modify an object defined with a const-qualified > > > type through use of an lvalue with non-const-qualified type, the > > > behavior is undefined. > > > > > > LLVM is removing such assignments. A simple fix is to not declare > > > variables const that you plan on modifying. Limiting the scope would be > > > a better method of preventing unwanted writes to such a variable. > > This is now documented in the LLVM release notes for Clang-9: > https://github.com/llvm/llvm-project/commit/e39e79358fcdd5d8ad809defaa821f0bbfa809a5 > > > > > > > Further, the code in question mentions "compiler bugs" without any links > > > to bug reports, so it is difficult to know if the issue is resolved in > > > GCC. The patch was authored in 2006, which would have been GCC 4.0.3 or > > > 4.1.1. The minimal supported version of GCC in the Linux kernel is > > > currently 4.6. > > > > It's somewhat older than that. My investigation points to: > > > > commit c94e57dcd61d661749d53ee876ab265883b0a103 > > Author: Ralf Baechle <ralf@linux-mips.org> > > Date: Sun Nov 25 09:25:53 2001 +0000 > > > > Cleanup of include/asm-mips/io.h. Now looks neat and harmless. > > Oh indeed, great find! > > So it looks to me like the order of events is: > 1. https://github.com/jaaron/linux-mips-ip30/commit/c94e57dcd61d661749d53ee876ab265883b0a103 > in 2001 first introduces the UB. mips_io_port_base is defined > non-const in arch/mips/kernel/setup.c, but then declared extern const > (and modified via UB) in include/asm-mips/io.h. A setter is created, > but not a getter (I'll revisit this below). This appears to work (due > to luck) for a few years until: > 2. https://github.com/mpe/linux-fullhistory/commit/966f4406d903a4214fdc74bec54710c6232a95b8 > in 2006 adds a compiler barrier (reload all variables) and this > appears to work. The commit message mentions that reads after > modification of the const variable were buggy (likely GCC started > taking advantage of the explicit UB around this time as well). This > isn't a fix for UB (more thoughts below), but appears to work. > 3. https://github.com/llvm/llvm-project/commit/b45631090220b732e614b5530bbd1d230eb9d38e > in 2019 removes writes to const variables in LLVM as that's explicit > UB. We observe the boot failure in mips and narrow it down to this > instance. > > I can see how throwing a compiler barrier in there made subsequent > reads after UB writes appear to work, but that was more due to luck > and implementation details of GCC than the heart of the issue (ie. not > writing code that is explicitly undefined behavior)(and could change > in future versions of GCC). Stated another way, the fix for explicit > UB is not hacks, but avoiding the UB by rewriting the problematic > code. > > > However the purpose of the arrangement does not appear to me to be > > particularly specific to a compiler version. > > > > > For what its worth, there was UB before the commit in question, it just > > > added a barrier and got lucky IRT codegen. I don't think there's any > > > actual compiler bugs related, just runtime bugs due to UB. > > > > Does your solution preserves the original purpose of the hack though as > > documented in the comment you propose to be removed? > > The function modified simply writes to a global variable. It's not > clear to my why the value about to be modified would EVER be loaded > before modification. > > > Clearly it was defined enough to work for almost 18 years, so it would be > > good to keep the optimisation functionally by using different means that > > do not rely on UB. > > "Defined enough" ??? > https://youtu.be/Aq_1l316ow8?t=17 > > > This variable is assigned at most once throughout the > > life of the kernel and then early on, so considering it r/w with all the > > consequences for all accesses does not appear to me to be a good use of > > it. > > Note: it's not possible to express the semantics of a "write once > variable" in C short of static initialization (AFAIK, without explicit > violation of UB, but Cunningham's Law may apply). > > (set_io_port_base is called in ~20 places) > > Thinking more about this while I was away, I think what this code has > needed since 2001 is proper encapsulation. If you want a variable > that is written from one place only, but readable throughout, then the > pattern I'd use is: > > 1. declare a getter in a .h file. > 2. define/qualify `mips_io_port_base` as `static` and non-const in a > .c file where it's modified. > 3. define the getter and setter in the above .c file. > > That would rely on linkage to limit the visibility of the symbol for > modification. But, we'd then need to export the getter, vs the symbol > itself. There's also on the order of ~20 call sites that would need > to be changed to invoke the getter rather than read the raw variable. > Also, it's unlikely the getter gets inlined across translation units > (short of LTO, which the mainline kernel doesn't support today). > > I think my patch here (https://lkml.org/lkml/2019/7/29/1636) is > minimally and much less invasive. > > > Maybe a piece of inline asm to hide the initialisation or suchlike then? > > I think that would still be UB as the definition would not be changed; > you'd still be modifying a variable declared const. > -- > Thanks, > ~Nick Desaulniers -- Thanks, ~Nick Desaulniers
On Tue, Aug 20, 2019 at 10:15 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > Hi Paul, > Bumping this thread; we'd really like to be able to boot test another > ISA in our CI. This lone patch is affecting our ability to boot. Can > you please pick it up? > https://lore.kernel.org/lkml/20190729211014.39333-1-ndesaulniers@google.com/ Hi Paul, Following up with this link that explains the undefined behavior issue more: https://wiki.sei.cmu.edu/confluence/display/c/EXP05-C.+Do+not+cast+away+a+const+qualification Please reconsider accepting this patch. > > On Wed, Aug 7, 2019 at 2:12 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > Sorry for the delayed response, literally sent the patch then went on vacation. > > > > On Mon, Jul 29, 2019 at 3:16 PM Maciej W. Rozycki <macro@linux-mips.org> wrote: > > > > > > On Mon, 29 Jul 2019, Nick Desaulniers wrote: > > > > > > > The code in question is modifying a variable declared const through > > > > pointer manipulation. Such code is explicitly undefined behavior, and > > > > is the lone issue preventing malta_defconfig from booting when built > > > > with Clang: > > > > > > > > If an attempt is made to modify an object defined with a const-qualified > > > > type through use of an lvalue with non-const-qualified type, the > > > > behavior is undefined. > > > > > > > > LLVM is removing such assignments. A simple fix is to not declare > > > > variables const that you plan on modifying. Limiting the scope would be > > > > a better method of preventing unwanted writes to such a variable. > > > > This is now documented in the LLVM release notes for Clang-9: > > https://github.com/llvm/llvm-project/commit/e39e79358fcdd5d8ad809defaa821f0bbfa809a5 > > > > > > > > > > Further, the code in question mentions "compiler bugs" without any links > > > > to bug reports, so it is difficult to know if the issue is resolved in > > > > GCC. The patch was authored in 2006, which would have been GCC 4.0.3 or > > > > 4.1.1. The minimal supported version of GCC in the Linux kernel is > > > > currently 4.6. > > > > > > It's somewhat older than that. My investigation points to: > > > > > > commit c94e57dcd61d661749d53ee876ab265883b0a103 > > > Author: Ralf Baechle <ralf@linux-mips.org> > > > Date: Sun Nov 25 09:25:53 2001 +0000 > > > > > > Cleanup of include/asm-mips/io.h. Now looks neat and harmless. > > > > Oh indeed, great find! > > > > So it looks to me like the order of events is: > > 1. https://github.com/jaaron/linux-mips-ip30/commit/c94e57dcd61d661749d53ee876ab265883b0a103 > > in 2001 first introduces the UB. mips_io_port_base is defined > > non-const in arch/mips/kernel/setup.c, but then declared extern const > > (and modified via UB) in include/asm-mips/io.h. A setter is created, > > but not a getter (I'll revisit this below). This appears to work (due > > to luck) for a few years until: > > 2. https://github.com/mpe/linux-fullhistory/commit/966f4406d903a4214fdc74bec54710c6232a95b8 > > in 2006 adds a compiler barrier (reload all variables) and this > > appears to work. The commit message mentions that reads after > > modification of the const variable were buggy (likely GCC started > > taking advantage of the explicit UB around this time as well). This > > isn't a fix for UB (more thoughts below), but appears to work. > > 3. https://github.com/llvm/llvm-project/commit/b45631090220b732e614b5530bbd1d230eb9d38e > > in 2019 removes writes to const variables in LLVM as that's explicit > > UB. We observe the boot failure in mips and narrow it down to this > > instance. > > > > I can see how throwing a compiler barrier in there made subsequent > > reads after UB writes appear to work, but that was more due to luck > > and implementation details of GCC than the heart of the issue (ie. not > > writing code that is explicitly undefined behavior)(and could change > > in future versions of GCC). Stated another way, the fix for explicit > > UB is not hacks, but avoiding the UB by rewriting the problematic > > code. > > > > > However the purpose of the arrangement does not appear to me to be > > > particularly specific to a compiler version. > > > > > > > For what its worth, there was UB before the commit in question, it just > > > > added a barrier and got lucky IRT codegen. I don't think there's any > > > > actual compiler bugs related, just runtime bugs due to UB. > > > > > > Does your solution preserves the original purpose of the hack though as > > > documented in the comment you propose to be removed? > > > > The function modified simply writes to a global variable. It's not > > clear to my why the value about to be modified would EVER be loaded > > before modification. > > > > > Clearly it was defined enough to work for almost 18 years, so it would be > > > good to keep the optimisation functionally by using different means that > > > do not rely on UB. > > > > "Defined enough" ??? > > https://youtu.be/Aq_1l316ow8?t=17 > > > > > This variable is assigned at most once throughout the > > > life of the kernel and then early on, so considering it r/w with all the > > > consequences for all accesses does not appear to me to be a good use of > > > it. > > > > Note: it's not possible to express the semantics of a "write once > > variable" in C short of static initialization (AFAIK, without explicit > > violation of UB, but Cunningham's Law may apply). > > > > (set_io_port_base is called in ~20 places) > > > > Thinking more about this while I was away, I think what this code has > > needed since 2001 is proper encapsulation. If you want a variable > > that is written from one place only, but readable throughout, then the > > pattern I'd use is: > > > > 1. declare a getter in a .h file. > > 2. define/qualify `mips_io_port_base` as `static` and non-const in a > > .c file where it's modified. > > 3. define the getter and setter in the above .c file. > > > > That would rely on linkage to limit the visibility of the symbol for > > modification. But, we'd then need to export the getter, vs the symbol > > itself. There's also on the order of ~20 call sites that would need > > to be changed to invoke the getter rather than read the raw variable. > > Also, it's unlikely the getter gets inlined across translation units > > (short of LTO, which the mainline kernel doesn't support today). > > > > I think my patch here (https://lkml.org/lkml/2019/7/29/1636) is > > minimally and much less invasive. > > > > > Maybe a piece of inline asm to hide the initialisation or suchlike then? > > > > I think that would still be UB as the definition would not be changed; > > you'd still be modifying a variable declared const. > > -- > > Thanks, > > ~Nick Desaulniers > > > > -- > Thanks, > ~Nick Desaulniers -- Thanks, ~Nick Desaulniers
Hi Nick, On Fri, Aug 23, 2019 at 10:16:04AM -0700, Nick Desaulniers wrote: > On Tue, Aug 20, 2019 at 10:15 AM Nick Desaulniers > <ndesaulniers@google.com> wrote: > > Hi Paul, > > Bumping this thread; we'd really like to be able to boot test another > > ISA in our CI. This lone patch is affecting our ability to boot. Can > > you please pick it up? > > https://lore.kernel.org/lkml/20190729211014.39333-1-ndesaulniers@google.com/ > > Hi Paul, > Following up with this link that explains the undefined behavior issue more: > https://wiki.sei.cmu.edu/confluence/display/c/EXP05-C.+Do+not+cast+away+a+const+qualification > Please reconsider accepting this patch. Sorry, it's been a crazy few months & I'm currently away awaiting my father's funeral so I'm working through a backlog & catching up on things. It will be a shame to lose the optimization opportunities const offers us, but it is an ugly hack & so I'm OK with applying this. It's likely to affect older machines more than newer ones (which tend to use less or no I/O port access) so I'm not too worried about the impact, but if we find it matters we can always try the fixmap approach I suggested previously. Thanks, Paul
Hello, Nick Desaulniers wrote: > The code in question is modifying a variable declared const through > pointer manipulation. Such code is explicitly undefined behavior, and > is the lone issue preventing malta_defconfig from booting when built > with Clang: > > If an attempt is made to modify an object defined with a const-qualified > type through use of an lvalue with non-const-qualified type, the > behavior is undefined. > > LLVM is removing such assignments. A simple fix is to not declare > variables const that you plan on modifying. Limiting the scope would be > a better method of preventing unwanted writes to such a variable. > > Further, the code in question mentions "compiler bugs" without any links > to bug reports, so it is difficult to know if the issue is resolved in > GCC. The patch was authored in 2006, which would have been GCC 4.0.3 or > 4.1.1. The minimal supported version of GCC in the Linux kernel is > currently 4.6. > > For what its worth, there was UB before the commit in question, it just > added a barrier and got lucky IRT codegen. I don't think there's any > actual compiler bugs related, just runtime bugs due to UB. > > Fixes: 966f4406d903 ("[MIPS] Work around bad code generation for <asm/io.h>.") Applied to mips-next. > commit 12051b318bc3 > https://git.kernel.org/mips/c/12051b318bc3 > > Link: https://github.com/ClangBuiltLinux/linux/issues/610 > Fixes: 966f4406d903 ("[MIPS] Work around bad code generation for <asm/io.h>.") > Reported-by: Nathan Chancellor <natechancellor@gmail.com> > Debugged-by: Nathan Chancellor <natechancellor@gmail.com> > Suggested-by: Eli Friedman <efriedma@quicinc.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > Tested-by: Nathan Chancellor <natechancellor@gmail.com> > Signed-off-by: Paul Burton <paul.burton@mips.com> Thanks, Paul [ This message was auto-generated; if you believe anything is incorrect then please email paul.burton@mips.com to report it. ]
On Sat, Aug 24, 2019 at 7:12 AM Paul Burton <paul.burton@mips.com> wrote: > > Hi Nick, > > On Fri, Aug 23, 2019 at 10:16:04AM -0700, Nick Desaulniers wrote: > > On Tue, Aug 20, 2019 at 10:15 AM Nick Desaulniers > > <ndesaulniers@google.com> wrote: > > > Hi Paul, > > > Bumping this thread; we'd really like to be able to boot test another > > > ISA in our CI. This lone patch is affecting our ability to boot. Can > > > you please pick it up? > > > https://lore.kernel.org/lkml/20190729211014.39333-1-ndesaulniers@google.com/ > > > > Hi Paul, > > Following up with this link that explains the undefined behavior issue more: > > https://wiki.sei.cmu.edu/confluence/display/c/EXP05-C.+Do+not+cast+away+a+const+qualification > > Please reconsider accepting this patch. > > Sorry, it's been a crazy few months & I'm currently away awaiting my > father's funeral so I'm working through a backlog & catching up on > things. That's an extremely tough hand to be dealt. Got it myself a week before I turned 20. Technically, (spoiler) everyone is eventually dealt it; not that that or really anything else can ever truly provide solace despite the common refrain "time heals all wounds" (Narrator: it doesn't). Depending on where you are in life it can really really destabilize things. I wrote this blog post (https://nickdesaulniers.github.io/blog/2013/04/29/the-persistence-of-memory/) that I'm not too proud of at a time I now realize that I was still kind of a state of shock (even years later). I also found it very difficult to accept advice from others who didn't share the experience of losing a parent. No one would fault you for asking your co-maintainers to handle more maintainer responsibilities for a while. I hope you can find a friend to commiserate with over beers. I owe you one. > > It will be a shame to lose the optimization opportunities const offers > us, but it is an ugly hack & so I'm OK with applying this. It's likely > to affect older machines more than newer ones (which tend to use less or > no I/O port access) so I'm not too worried about the impact, but if we > find it matters we can always try the fixmap approach I suggested > previously. > > Thanks, > Paul -- Thanks, ~Nick Desaulniers
diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h index 97a280640daf..d58ff2229738 100644 --- a/arch/mips/include/asm/io.h +++ b/arch/mips/include/asm/io.h @@ -63,21 +63,11 @@ * instruction, so the lower 16 bits must be zero. Should be true on * on any sane architecture; generic code does not use this assumption. */ -extern const unsigned long mips_io_port_base; +extern unsigned long mips_io_port_base; -/* - * Gcc will generate code to load the value of mips_io_port_base after each - * function call which may be fairly wasteful in some cases. So we don't - * play quite by the book. We tell gcc mips_io_port_base is a long variable - * which solves the code generation issue. Now we need to violate the - * aliasing rules a little to make initialization possible and finally we - * will need the barrier() to fight side effects of the aliasing chat. - * This trickery will eventually collapse under gcc's optimizer. Oh well. - */ static inline void set_io_port_base(unsigned long base) { - * (unsigned long *) &mips_io_port_base = base; - barrier(); + mips_io_port_base = base; } /* diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c index ab349d2381c3..675223a66d0c 100644 --- a/arch/mips/kernel/setup.c +++ b/arch/mips/kernel/setup.c @@ -76,7 +76,7 @@ static char __initdata builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE; * mips_io_port_base is the begin of the address space to which x86 style * I/O ports are mapped. */ -const unsigned long mips_io_port_base = -1; +unsigned long mips_io_port_base = -1; EXPORT_SYMBOL(mips_io_port_base); static struct resource code_resource = { .name = "Kernel code", };
The code in question is modifying a variable declared const through pointer manipulation. Such code is explicitly undefined behavior, and is the lone issue preventing malta_defconfig from booting when built with Clang: If an attempt is made to modify an object defined with a const-qualified type through use of an lvalue with non-const-qualified type, the behavior is undefined. LLVM is removing such assignments. A simple fix is to not declare variables const that you plan on modifying. Limiting the scope would be a better method of preventing unwanted writes to such a variable. Further, the code in question mentions "compiler bugs" without any links to bug reports, so it is difficult to know if the issue is resolved in GCC. The patch was authored in 2006, which would have been GCC 4.0.3 or 4.1.1. The minimal supported version of GCC in the Linux kernel is currently 4.6. For what its worth, there was UB before the commit in question, it just added a barrier and got lucky IRT codegen. I don't think there's any actual compiler bugs related, just runtime bugs due to UB. Link: https://github.com/ClangBuiltLinux/linux/issues/610 Fixes: 966f4406d903 ("[MIPS] Work around bad code generation for <asm/io.h>.") Reported-by: Nathan Chancellor <natechancellor@gmail.com> Debugged-by: Nathan Chancellor <natechancellor@gmail.com> Suggested-by: Eli Friedman <efriedma@quicinc.com> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- arch/mips/include/asm/io.h | 14 ++------------ arch/mips/kernel/setup.c | 2 +- 2 files changed, 3 insertions(+), 13 deletions(-) -- 2.22.0.709.g102302147b-goog