Messages in this thread Patch in this message | | | From | Linus Torvalds <> | Date | Mon, 15 Nov 2010 08:04:49 -0800 | Subject | Re: gcc 4.5.1 / as 2.20.51.0.11 miscompiling drivers/char/i8k.c ? |
| |
On Mon, Nov 15, 2010 at 3:16 AM, Jakub Jelinek <jakub@redhat.com> wrote: > > I don't see any problems on the assembly level. i8k_smm is > not inlined in this case and checks all 3 conditions.
If it really is related to gcc not understanding that "*regs" has changed due to the memory being an automatic variable, and passing in "regs" itself as a pointer to that automatic variable together with the "memory" clobber not being sufficient, than I think it's the lack of inlining that will automatically hide the bug.
(Side note: and I think this does show how much of a gcc bug it is not to consider "memory" together with passing in a pointer to an asm to always be a clobber).
Because if it isn't inlined, then "regs" will be seen a a real pointer to some external memory (the caller) rather than being optimized to just be the auto structure on the stack. Because *mem is auto only within the context of the caller.
Which actually points to a possible simpler: - remove the "+m" since it adds too much register pressure - mark the i8k_smm() as "noinline" instead.
Quite frankly, I'd hate to add even more crud to that inline asm (to save/restore the registers manually). It's already not the prettiest thing around.
So does the attached patch work for everybody?
Linus drivers/char/i8k.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c index f0863be..101011e 100644 --- a/drivers/char/i8k.c +++ b/drivers/char/i8k.c @@ -114,7 +114,7 @@ static inline const char *i8k_get_dmi_data(int field) /* * Call the System Management Mode BIOS. Code provided by Jonathan Buzzard. */ -static int i8k_smm(struct smm_regs *regs) +static noinline int i8k_smm(struct smm_regs *regs) { int rc; int eax = regs->eax; @@ -142,7 +142,7 @@ static int i8k_smm(struct smm_regs *regs) "lahf\n\t" "shrl $8,%%eax\n\t" "andl $1,%%eax\n" - :"=a"(rc), "+m" (*regs) + :"=a"(rc) : "a"(regs) : "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory"); #else @@ -168,7 +168,7 @@ static int i8k_smm(struct smm_regs *regs) "lahf\n\t" "shrl $8,%%eax\n\t" "andl $1,%%eax\n" - :"=a"(rc), "+m" (*regs) + :"=a"(rc) : "a"(regs) : "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory"); #endif | |