On Wed, Feb 7, 2024 at 2:55 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> On Wed, Feb 07, 2024 at 10:22:48PM +0000, Al Viro wrote:
> > One way to do that would be to replace d_name with
> > union {
> > const struct qstr d_name;
> > struct qstr __d_name;
> > };
> > and let the the places that want to modify it use __d_name.
> > Tempting, but the thing that makes me rather nervous about this
> > approach is that such games with unions are straying into
> > the nasal demon country (C99 6.7.3[5]), inviting the optimizers
> > to play. Matt Wilcox pointed out that mainline already has
> > i_nlink/__i_nlink pair, but... there are fewer users of those
> > and the damage from miscompiles would be less sensitive.
> > Patch along those lines would be pretty simple, though.
>
> You're referring to this, I assume:
>
> 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
I have 100% observed llvm throw out writes to objects declared as
const where folks tried to write via "casting away the const" (since
that's UB) which resulted in boot failures in the Linux kernel
affecting android devices. I can't find the commit in question at the
moment, but seemed to have made some kind of note in it in 2020.
https://android-review.git.corp.google.com/c/platform/prebuilts/clang/host/linux-x86/+/1201901/1/RELEASE_NOTES.md
That said, I just tried Al's union, and don't observe such optimizations.
https://godbolt.org/z/zrj71E8W5
This whole suggestion makes me laugh "in C++ private members."
>
> I'm not sure that's relevant. As I see it, we're defining two objects,
> one const-qualified and one not. The union specifies that they share
> the same storage ("a union is a type consisting of a sequence of members
> whose storage overlap").
>
> I see 6.7.3 as saying "even if you cast away the const from d_name,
> modifyig d_name is undefined", but you're not modifying d_name,
> you're modifying __d_name, which just happens to share storage with
> d_name.
>
> I think 6.7.2.1[14] is more likely to cause us problems:
>
> The value of at most one of the members can be stored in a union
> object at any time.
>
> From that the compiler can assume that if it sees a store to __d_name
> that accesses to d_name are undefined? Perhaps? My brain always starts
> to hurt when people start spec-lawyering.
>
> > * add an inlined helper,
> > static inline const struct qstr *d_name(const struct dentry *d)
> > {
> > return &d->d_name;
> > }
>
> I'm in no way opposed to this solution. I just thought that the i_nlink
> solution might also work for you (and if the compiler people say the
> i_nlink solution is actually not spec compliant, then I guess we can
> adopt an i_nlink_read() function).
--
Thanks,
~Nick Desaulniers
