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-ndesaulni...@google.com/
On Wed, Aug 7, 2019 at 2:12 PM Nick Desaulniers <ndesaulni...@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 <ma...@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 <r...@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