SGkgQm9heiwNCg0KV2hlbiBJIHJ1biAnc3BhcnNlJyBvbiB0aGUgb2JqZWN0IGxheW91dCBkcml2
ZXIsIEkgZ2V0IHRoZSBmb2xsb3dpbmcNCmVycm9yczoNCg0KZnMvbmZzL29iamxheW91dC9vYmpp
b19vc2QuYzoyMTA6Mzc6IGVycm9yOiBiYWQgY29uc3RhbnQgZXhwcmVzc2lvbg0KZnMvbmZzL29i
amxheW91dC9vYmppb19vc2QuYzoyMTE6Mzk6IGVycm9yOiBiYWQgY29uc3RhbnQgZXhwcmVzc2lv
bg0KZnMvbmZzL29iamxheW91dC9vYmppb19vc2QuYzozMTU6NTk6IGVycm9yOiBiYWQgY29uc3Rh
bnQgZXhwcmVzc2lvbg0KDQp3aGljaCBib2lscyBkb3duIHRvIHRoZSBmb2xsb3dpbmcgc2VjdGlv
biBvZiBjb2RlOg0KDQppbnQgX19hbGxvY19vYmppb19zZWcodW5zaWduZWQgbnVtZGV2cywgZ2Zw
X3QgZ2ZwX2ZsYWdzLA0KICAgICAgICAgICAgICAgICAgICAgICBzdHJ1Y3Qgb2JqaW9fc2VnbWVu
dCAqKnBzZWcpDQp7DQogICAgICAgIHN0cnVjdCBfX2FsbG9jX29iamlvX3NlZ21lbnQgew0KICAg
ICAgICAgICAgICAgIHN0cnVjdCBvYmppb19zZWdtZW50IG9sc2VnOw0KICAgICAgICAgICAgICAg
IHN0cnVjdCBvcmVfZGV2ICpvZHNbbnVtZGV2c107DQoJCV5eXl5eXl5eXl5eXl5eXl5eXl5eXl5e
Xl5eXl5eDQogICAgICAgICAgICAgICAgc3RydWN0IG9yZV9jb21wIGNvbXBzW251bWRldnNdOw0K
CQleXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eDQogICAgICAgIH0gKmFvbHNlZzsNCg0K
ICAgICAgICBhb2xzZWcgPSBremFsbG9jKHNpemVvZigqYW9sc2VnKSwgZ2ZwX2ZsYWdzKTsNCg0K
DQpEeW5hbWljYWxseSBhbGxvY2F0ZWQgYXJyYXlzIHN1Y2ggYXMgdGhlIGFib3ZlIGFyZSBhIEdO
VSBDIGNvbXBpbGVyDQpmZWF0dXJlIHRoYXQgd2UgY2Fubm90IHVzZSBpbiBrZXJuZWwgY29kZS4g
SWYgdGhlIGFib3ZlIGFycmF5IHNpemVzIG5lZWQNCnRvIGJlIGR5bmFtaWNhbGx5IGRldGVybWlu
ZWQsIHRoZW4gcGxlYXNlIHVzZSB0aGUga2NhbGxvYygpIGludGVyZmFjZSB0bw0KYWxsb2NhdGUg
dGhlbS4NCkkuZS4gc29tZXRoaW5nIGxpa2UNCg0KICAgICAgICBzdHJ1Y3QgX19hbGxvY19vYmpp
b19zZWdtZW50IHsNCiAgICAgICAgICAgICAgICBzdHJ1Y3Qgb2JqaW9fc2VnbWVudCBvbHNlZzsN
CiAgICAgICAgICAgICAgICBzdHJ1Y3Qgb3JlX2RldiAqKm9kczsNCiAgICAgICAgICAgICAgICBz
dHJ1Y3Qgb3JlX2NvbXAgKmNvbXBzOw0KCQleXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5e
DQogICAgICAgIH0gKmFvbHNlZzsNCg0KICAgICAgICBhb2xzZWcgPSBremFsbG9jKHNpemVvZigq
YW9sc2VnKSwgZ2ZwX2ZsYWdzKTsNCglpZiAoYW9sc2VnKSB7DQoJCWFvbHNlZy0+b2RzID0ga2Nh
bGxvYyhudW1kZXZzLCBzaXplb2Yoc3RydWN0IG9yZV9kZXYgKiksIGdmcF9mbGFncyk7DQoJCWFv
bHNlZy0+b3JlX2NvbXAgPSBrY2FsbG9jKG51bWRldnMsIHNpemVvZihzdHJ1Y3Qgb3JlX2NvbXAp
LCBnZnBfZmxhZ3MpOw0KCQlpZiAoIWFvbHNlZy0+b2RzIHx8ICFhb2xzZWctPm9yZV9jb21wKQ0K
CQkJLi4uLiBjbGVhbiB1cCBhbmQgcmVwb3J0IGVycm9yLi4uDQoJfQ0KDQpDaGVlcnMNCiAgVHJv
bmQNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXINCg0K
TmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg0K
On 03/12/2012 02:52 PM, Myklebust, Trond wrote:
> On Mon, 2012-03-12 at 12:52 -0700, Boaz Harrosh wrote:
>> On 03/11/2012 10:37 AM, Myklebust, Trond wrote:
>>> Hi Boaz,
>>>
>>> When I run 'sparse' on the object layout driver, I get the following
>>> errors:
>>>
>>> fs/nfs/objlayout/objio_osd.c:210:37: error: bad constant expression
>>> fs/nfs/objlayout/objio_osd.c:211:39: error: bad constant expression
>>> fs/nfs/objlayout/objio_osd.c:315:59: error: bad constant expression
>>>
>>
>> Then sparse is broken
>>
>>> which boils down to the following section of code:
>>>
>>> int __alloc_objio_seg(unsigned numdevs, gfp_t gfp_flags,
>>> struct objio_segment **pseg)
>>> {
>>> struct __alloc_objio_segment {
>>> struct objio_segment olseg;
>>> struct ore_dev *ods[numdevs];
>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> struct ore_comp comps[numdevs];
>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> } *aolseg;
>>>
>>> aolseg = kzalloc(sizeof(*aolseg), gfp_flags);
>>>
>>>
>>> Dynamically allocated arrays such as the above are a GNU C compiler
>>> feature that we cannot use in kernel code. If the above array sizes need
>>> to be dynamically determined, then please use the kcalloc() interface to
>>> allocate them.
>>
>> I am using kzalloc to do a single allocation. The *ods[numdevs] is only
>> used for the sizeof() expression it does not allocate anything actually.
>>
>> We use gcc extensions all over the place. From the simple __func__ to
>> the famous typeof().
>
> __func__ is actually valid C99.
>
>> I don't understand why you decide to pick on this one in particular.
>
> It has been shown to be troublesome previously. See for instance
>
> http://lkml.org/lkml/2011/10/23/25
>
Again I do not actually use the Compiler generated code at all and I do
not use/access this struct in any way. All I do is use it's sizeof()
calculation which just simply does the right thing. Actually all it
does, I checked the assembly multiple times, is exactly the below
+ numdevs * sizeof(struct ore_dev *) thing
> Then there is the issue that alternative compilers such as clang barf
> all over it.
>
Is that supported?
>>> I.e. something like
>>>
>>> struct __alloc_objio_segment {
>>> struct objio_segment olseg;
>>> struct ore_dev **ods;
>>> struct ore_comp *comps;
>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> } *aolseg;
>>>
>>> aolseg = kzalloc(sizeof(*aolseg), gfp_flags);
>>> if (aolseg) {
>>> aolseg->ods = kcalloc(numdevs, sizeof(struct ore_dev *), gfp_flags);
>>> aolseg->ore_comp = kcalloc(numdevs, sizeof(struct ore_comp), gfp_flags);
>>
>> This is not the same code. It is three allocations instead of one. And it is an
>> extra pointer reference instead of just pointer offsetting.
>>
>> If it would change it would change to:
>>
>> aolseg = kzalloc(sizeof(*aolseg) +
>> numdevs * sizeof(struct ore_dev *),
>> numdevs * sizeof(struct ore_comp),
>> gfp_flags);
>
> That is broken, since it doesn't take into account alignment issues
> within the structure.
That's not true. sizeof() will always, by definition, return aligned
size. .i.e It will return a byte size, such as, when in an array will
return the next element in the array. We use this pattern a lot in the
Kernel.
> I also don't see how you would express the above
> as a single structure without using variable length arrays.
>
That's easy. Actually that code used to be the old style for a long time
until recently when I moved it to ORE types. It is basically just the same
as I had before. only I like this style because of type safety. (No casts)
>> And all the pointer arithmetic that follow. But why the very ugly code
>> when there is an elegant, type-safe, and self documenting way. It's
>> beautifully clear this way what is the in memory structure of the
>> allocation.
>
> While it may look beautiful, it has been proven to be immature and to
> generate ugly code.
>
Again, not in my case. Because this structure is private to that function
which never accesses that structure. It is completely safe in all ARCHs.
But that said. I'm not going to fight it. Give me 1/2 a day and I'll send
you a fixing (uglifying) patch
Sigh
Boaz
T24gTW9uLCAyMDEyLTAzLTEyIGF0IDEyOjUyIC0wNzAwLCBCb2F6IEhhcnJvc2ggd3JvdGU6DQo+
IE9uIDAzLzExLzIwMTIgMTA6MzcgQU0sIE15a2xlYnVzdCwgVHJvbmQgd3JvdGU6DQo+ID4gSGkg
Qm9heiwNCj4gPiANCj4gPiBXaGVuIEkgcnVuICdzcGFyc2UnIG9uIHRoZSBvYmplY3QgbGF5b3V0
IGRyaXZlciwgSSBnZXQgdGhlIGZvbGxvd2luZw0KPiA+IGVycm9yczoNCj4gPiANCj4gPiBmcy9u
ZnMvb2JqbGF5b3V0L29iamlvX29zZC5jOjIxMDozNzogZXJyb3I6IGJhZCBjb25zdGFudCBleHBy
ZXNzaW9uDQo+ID4gZnMvbmZzL29iamxheW91dC9vYmppb19vc2QuYzoyMTE6Mzk6IGVycm9yOiBi
YWQgY29uc3RhbnQgZXhwcmVzc2lvbg0KPiA+IGZzL25mcy9vYmpsYXlvdXQvb2JqaW9fb3NkLmM6
MzE1OjU5OiBlcnJvcjogYmFkIGNvbnN0YW50IGV4cHJlc3Npb24NCj4gPiANCj4gDQo+IFRoZW4g
c3BhcnNlIGlzIGJyb2tlbg0KPiANCj4gPiB3aGljaCBib2lscyBkb3duIHRvIHRoZSBmb2xsb3dp
bmcgc2VjdGlvbiBvZiBjb2RlOg0KPiA+IA0KPiA+IGludCBfX2FsbG9jX29iamlvX3NlZyh1bnNp
Z25lZCBudW1kZXZzLCBnZnBfdCBnZnBfZmxhZ3MsDQo+ID4gICAgICAgICAgICAgICAgICAgICAg
ICBzdHJ1Y3Qgb2JqaW9fc2VnbWVudCAqKnBzZWcpDQo+ID4gew0KPiA+ICAgICAgICAgc3RydWN0
IF9fYWxsb2Nfb2JqaW9fc2VnbWVudCB7DQo+ID4gICAgICAgICAgICAgICAgIHN0cnVjdCBvYmpp
b19zZWdtZW50IG9sc2VnOw0KPiA+ICAgICAgICAgICAgICAgICBzdHJ1Y3Qgb3JlX2RldiAqb2Rz
W251bWRldnNdOw0KPiA+IAkJXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl4NCj4gPiAgICAg
ICAgICAgICAgICAgc3RydWN0IG9yZV9jb21wIGNvbXBzW251bWRldnNdOw0KPiA+IAkJXl5eXl5e
Xl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXg0KPiA+ICAgICAgICAgfSAqYW9sc2VnOw0KPiA+IA0K
PiA+ICAgICAgICAgYW9sc2VnID0ga3phbGxvYyhzaXplb2YoKmFvbHNlZyksIGdmcF9mbGFncyk7
DQo+ID4gDQo+ID4gDQo+ID4gRHluYW1pY2FsbHkgYWxsb2NhdGVkIGFycmF5cyBzdWNoIGFzIHRo
ZSBhYm92ZSBhcmUgYSBHTlUgQyBjb21waWxlcg0KPiA+IGZlYXR1cmUgdGhhdCB3ZSBjYW5ub3Qg
dXNlIGluIGtlcm5lbCBjb2RlLiBJZiB0aGUgYWJvdmUgYXJyYXkgc2l6ZXMgbmVlZA0KPiA+IHRv
IGJlIGR5bmFtaWNhbGx5IGRldGVybWluZWQsIHRoZW4gcGxlYXNlIHVzZSB0aGUga2NhbGxvYygp
IGludGVyZmFjZSB0bw0KPiA+IGFsbG9jYXRlIHRoZW0uDQo+IA0KPiBJIGFtIHVzaW5nIGt6YWxs
b2MgdG8gZG8gYSBzaW5nbGUgYWxsb2NhdGlvbi4gVGhlICpvZHNbbnVtZGV2c10gaXMgb25seQ0K
PiB1c2VkIGZvciB0aGUgc2l6ZW9mKCkgZXhwcmVzc2lvbiBpdCBkb2VzIG5vdCBhbGxvY2F0ZSBh
bnl0aGluZyBhY3R1YWxseS4NCj4gDQo+IFdlIHVzZSBnY2MgZXh0ZW5zaW9ucyBhbGwgb3ZlciB0
aGUgcGxhY2UuIEZyb20gdGhlIHNpbXBsZSBfX2Z1bmNfXyB0bw0KPiB0aGUgZmFtb3VzIHR5cGVv
ZigpLg0KDQpfX2Z1bmNfXyBpcyBhY3R1YWxseSB2YWxpZCBDOTkuDQoNCj4gSSBkb24ndCB1bmRl
cnN0YW5kIHdoeSB5b3UgZGVjaWRlIHRvIHBpY2sgb24gdGhpcyBvbmUgaW4gcGFydGljdWxhci4N
Cg0KSXQgaGFzIGJlZW4gc2hvd24gdG8gYmUgdHJvdWJsZXNvbWUgcHJldmlvdXNseS4gU2VlIGZv
ciBpbnN0YW5jZQ0KDQpodHRwOi8vbGttbC5vcmcvbGttbC8yMDExLzEwLzIzLzI1DQoNClRoZW4g
dGhlcmUgaXMgdGhlIGlzc3VlIHRoYXQgYWx0ZXJuYXRpdmUgY29tcGlsZXJzIHN1Y2ggYXMgY2xh
bmcgYmFyZg0KYWxsIG92ZXIgaXQuDQoNCj4gPiBJLmUuIHNvbWV0aGluZyBsaWtlDQo+ID4gDQo+
ID4gICAgICAgICBzdHJ1Y3QgX19hbGxvY19vYmppb19zZWdtZW50IHsNCj4gPiAgICAgICAgICAg
ICAgICAgc3RydWN0IG9iamlvX3NlZ21lbnQgb2xzZWc7DQo+ID4gICAgICAgICAgICAgICAgIHN0
cnVjdCBvcmVfZGV2ICoqb2RzOw0KPiA+ICAgICAgICAgICAgICAgICBzdHJ1Y3Qgb3JlX2NvbXAg
KmNvbXBzOw0KPiA+IAkJXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXg0KPiA+ICAgICAg
ICAgfSAqYW9sc2VnOw0KPiA+IA0KPiA+ICAgICAgICAgYW9sc2VnID0ga3phbGxvYyhzaXplb2Yo
KmFvbHNlZyksIGdmcF9mbGFncyk7DQo+ID4gCWlmIChhb2xzZWcpIHsNCj4gPiAJCWFvbHNlZy0+
b2RzID0ga2NhbGxvYyhudW1kZXZzLCBzaXplb2Yoc3RydWN0IG9yZV9kZXYgKiksIGdmcF9mbGFn
cyk7DQo+ID4gCQlhb2xzZWctPm9yZV9jb21wID0ga2NhbGxvYyhudW1kZXZzLCBzaXplb2Yoc3Ry
dWN0IG9yZV9jb21wKSwgZ2ZwX2ZsYWdzKTsNCj4gDQo+IFRoaXMgaXMgbm90IHRoZSBzYW1lIGNv
ZGUuIEl0IGlzIHRocmVlIGFsbG9jYXRpb25zIGluc3RlYWQgb2Ygb25lLiBBbmQgaXQgaXMgYW4N
Cj4gZXh0cmEgcG9pbnRlciByZWZlcmVuY2UgaW5zdGVhZCBvZiBqdXN0IHBvaW50ZXIgb2Zmc2V0
dGluZy4NCj4gDQo+IElmIGl0IHdvdWxkIGNoYW5nZSBpdCB3b3VsZCBjaGFuZ2UgdG86DQo+IA0K
PiAgICAgICAgICBhb2xzZWcgPSBremFsbG9jKHNpemVvZigqYW9sc2VnKSArIA0KPiAJCSAgICAg
ICAgICBudW1kZXZzICogc2l6ZW9mKHN0cnVjdCBvcmVfZGV2ICopLA0KPiAJCQkgIG51bWRldnMg
KiBzaXplb2Yoc3RydWN0IG9yZV9jb21wKSwgDQo+IAkJCSAgZ2ZwX2ZsYWdzKTsNCg0KVGhhdCBp
cyBicm9rZW4sIHNpbmNlIGl0IGRvZXNuJ3QgdGFrZSBpbnRvIGFjY291bnQgYWxpZ25tZW50IGlz
c3Vlcw0Kd2l0aGluIHRoZSBzdHJ1Y3R1cmUuIEkgYWxzbyBkb24ndCBzZWUgaG93IHlvdSB3b3Vs
ZCBleHByZXNzIHRoZSBhYm92ZQ0KYXMgYSBzaW5nbGUgc3RydWN0dXJlIHdpdGhvdXQgdXNpbmcg
dmFyaWFibGUgbGVuZ3RoIGFycmF5cy4NCg0KPiBBbmQgYWxsIHRoZSBwb2ludGVyIGFyaXRobWV0
aWMgdGhhdCBmb2xsb3cuIEJ1dCB3aHkgdGhlIHZlcnkgdWdseSBjb2RlDQo+IHdoZW4gdGhlcmUg
aXMgYW4gZWxlZ2FudCwgdHlwZS1zYWZlLCBhbmQgc2VsZiBkb2N1bWVudGluZyB3YXkuIEl0J3MN
Cj4gYmVhdXRpZnVsbHkgY2xlYXIgdGhpcyB3YXkgd2hhdCBpcyB0aGUgaW4gbWVtb3J5IHN0cnVj
dHVyZSBvZiB0aGUNCj4gYWxsb2NhdGlvbi4NCg0KV2hpbGUgaXQgbWF5IGxvb2sgYmVhdXRpZnVs
LCBpdCBoYXMgYmVlbiBwcm92ZW4gdG8gYmUgaW1tYXR1cmUgYW5kIHRvDQpnZW5lcmF0ZSB1Z2x5
IGNvZGUuDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWlu
ZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20N
Cg0K
On 03/11/2012 10:37 AM, Myklebust, Trond wrote:
> Hi Boaz,
>
> When I run 'sparse' on the object layout driver, I get the following
> errors:
>
> fs/nfs/objlayout/objio_osd.c:210:37: error: bad constant expression
> fs/nfs/objlayout/objio_osd.c:211:39: error: bad constant expression
> fs/nfs/objlayout/objio_osd.c:315:59: error: bad constant expression
>
Then sparse is broken
> which boils down to the following section of code:
>
> int __alloc_objio_seg(unsigned numdevs, gfp_t gfp_flags,
> struct objio_segment **pseg)
> {
> struct __alloc_objio_segment {
> struct objio_segment olseg;
> struct ore_dev *ods[numdevs];
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> struct ore_comp comps[numdevs];
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> } *aolseg;
>
> aolseg = kzalloc(sizeof(*aolseg), gfp_flags);
>
>
> Dynamically allocated arrays such as the above are a GNU C compiler
> feature that we cannot use in kernel code. If the above array sizes need
> to be dynamically determined, then please use the kcalloc() interface to
> allocate them.
I am using kzalloc to do a single allocation. The *ods[numdevs] is only
used for the sizeof() expression it does not allocate anything actually.
We use gcc extensions all over the place. From the simple __func__ to
the famous typeof().
I don't understand why you decide to pick on this one in particular.
> I.e. something like
>
> struct __alloc_objio_segment {
> struct objio_segment olseg;
> struct ore_dev **ods;
> struct ore_comp *comps;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> } *aolseg;
>
> aolseg = kzalloc(sizeof(*aolseg), gfp_flags);
> if (aolseg) {
> aolseg->ods = kcalloc(numdevs, sizeof(struct ore_dev *), gfp_flags);
> aolseg->ore_comp = kcalloc(numdevs, sizeof(struct ore_comp), gfp_flags);
This is not the same code. It is three allocations instead of one. And it is an
extra pointer reference instead of just pointer offsetting.
If it would change it would change to:
aolseg = kzalloc(sizeof(*aolseg) +
numdevs * sizeof(struct ore_dev *),
numdevs * sizeof(struct ore_comp),
gfp_flags);
And all the pointer arithmetic that follow. But why the very ugly code
when there is an elegant, type-safe, and self documenting way. It's
beautifully clear this way what is the in memory structure of the
allocation.
> if (!aolseg->ods || !aolseg->ore_comp)
> .... clean up and report error...
> }
>
Again, we use gcc extensions all over the place. Why not this one?
BTW variable arrays is also supported by M$-vcc.
> Cheers
> Trond
Thanks
Boaz