Re: [PATCH] x86: Fix compilation bug in kprobes' twobyte_is_boostable

[Josh Stone]

On 10/22/2011 05:25 PM, Linus Torvalds wrote:
> Hmm. I'd *much* rather do this in arch/x86/include/asm/bitops.h
> instead, methinks.

Agreed, that's why I tried that first.

> Also, rather than your
> 
>> See also my more general fix, https://lkml.org/lkml/2011/10/6/412
> 
> wouldn't the simple fix be just to add the volatile there to the cast
> we already do, ie something like the appended (cut-and-paste, so it's
> whitespace-damaged, but you get the idea).

Unfortunately, no.  Whatever const-propagation gcc is doing here
(somewhat wrongly per PR50571) is not affected by volatile on that cast.
 I also tried leaving it to the parameter type (no cast), no help.

Another historical note is that the parameter used to be void* until
commit 5136dea5, which I suppose is why the current cast exists at all.
 Reverting that still doesn't affect the immediate problem.

It may have gotten lost in the various messages I sent, but note that
this is only happening on i386, where the source u32 matches the size of
the casted long.  On x86_64, where long is of course bigger than u32,
the pointer aliasing seems to prevent the issue.  So yet another fix is
to make that asm cast something silly that will always alias, like
(*(char*)addr).  That might be even more magical/fragile though.

> And I don't mind volatile in code nearly as much as I mind volatile on
> the data structures (it's one of my "C typing was misdesigned" pet
> peeves: I think "volatile" is about the access, not about the data)

I just tried removing the const from twobyte_is_boostable[], and that
also does the trick.  Not sure why I didn't try that first -- I guess
because I saw all the volatiles in bitops.  Is that more palatable, or
would you still rather try to fix it in bitops.h directly?

>  - Long long ago, we had that "big array" approach for ADDR too. So
> we've wavered between the volatile and using a block memory op. But
> we've used the "volatile" for a long time now for the bit change ones,
> so I don't think we should mix concepts like your patch.

Do you recall why it settled on volatile?  That seems like the less
descriptive of the two approaches.  But "long long ago" appears to be
beyond recorded (git) history...

Thanks,
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/