r358919 causing boot failures for MIPS Linux kernel

[nickdesaulniers]

Bugzilla Link	42763
Resolution	INVALID
Resolved on	Aug 06, 2019 01:33
Version	trunk
OS	Linux
Blocks	#4440 #41819
CC	@davidbolvansky,@efriedma-quic,@zmodem,@preames,@nathanchance,@tstellar

Extended Description

Reported via: ClangBuiltLinux/linux#610 (comment)

It seems that with clang-8, we can boot a working MIPS linux kernel.

It fails to boot with clang-9, seemingly due to r358919 commit d748689 ("[InstCombine] Eliminate stores to constant memory").

The particular optimization sounds like it may be potentially dangerous; maybe the kernel is exercising such an unconsidered case?

[nickdesaulniers]

Can we please revert r358919 for the clang-9 release, then sort this out/reland for clang-10?

[zmodem]

+Philip who wrote r358919

[efriedma-quic]

I'd like to see a testcase before we decide to revert anything; given the nature of the change, it's likely we'll conclude the kernel code in question has undefined behavior.

[nickdesaulniers]

Via: ClangBuiltLinux/linux#610 (comment)

/*

• On MIPS I/O ports are memory mapped, so we access them using normal
• load/store instructions. mips_io_port_base is the virtual address to
• which all ports are being mapped. For sake of efficiency some code
• assumes that this is an address that can be loaded with a single lui
• 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;

/*

• 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();
    }

so I'm guessing that mips_io_port_base being declared const is problematic, but I'm not sure the LLVM change in question considers "casting away the const-ness."

[efriedma-quic]

You can't cast away the const-ness of a variable. If you try, it's undefined behavior. (From the C standard: "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.")

[nickdesaulniers]

0001-mips-avoid-undefined-behavior-in-assignment-of-mips_.patch
Thanks for the citation. I'm going to boot test this attached patch and see if it solves the issue for us.

[nickdesaulniers]

OK, sorry for the noise and thanks for helping debug. Nathan confirmed the attached patch allows us to boot. We'll send it upstream to the Linux kernel.

[davidbolvansky]

Maybe you should write about this optimization (and how to detect that UB.. sanitizers?) in Release notes?

[zmodem]

Maybe you should write about this optimization (and how to detect that UB..
sanitizers?) in Release notes?

+1 I was just about to suggest that.

Philip: would you be willing to mention this in the release notes?

[preames]

Maybe you should write about this optimization (and how to detect that UB..
sanitizers?) in Release notes?

+1 I was just about to suggest that.

Philip: would you be willing to mention this in the release notes?
Done in revision 367941. I went ahead and landed it so it didn't get forgotten, but if anyone has suggested improvements, let me know (or just land them).

[zmodem]

Maybe you should write about this optimization (and how to detect that UB..
sanitizers?) in Release notes?

+1 I was just about to suggest that.

Philip: would you be willing to mention this in the release notes?
Done in revision 367941. I went ahead and landed it so it didn't get
forgotten, but if anyone has suggested improvements, let me know (or just
land them).

Perfect, thanks! (I've moved the note to the release_90 branch in r367997.)