makesdna: Give suggestions for fix #110145

Ray molenkamp · 2023-07-15T20:10:04+02:00

Ray molenkamp commented

2023-07-15 20:10:04 +02:00

Give feedback on how many padding bytes are required and where to put
them.

Heads up : The code change is really irrelevant

It's a one liner, it's uncontroversial, it have comfortably landed
without going though the review process. The patch description isn't
very relevant to the problem it fixes but more of a general critique
on the overall state of makesdna. target audience : The Core Module

Background motivation

Someone on chat asked for help , and makesdna was being "unhelpful"
in my opion towards this dev, offering no useful suggestions how to
resolve their problem.

Repro

put an int oh_no_i_broke_things; just before the coba_weight field
in the UesrDef struct and you'll be met with the following "helpful"
information.

Generating dna.c, dna_type_offsets.h, dna_verify.c
CUSTOMBUILD : Align struct error : UserDef coba_weight
CUSTOMBUILD : Align struct error : UserDef walk_navigation
CUSTOMBUILD : Align struct error : UserDef space_data
CUSTOMBUILD : Align struct error : UserDef file_space_data
CUSTOMBUILD : Align struct error : UserDef experimental
CUSTOMBUILD : Align struct error : UserDef runtime
Sizeerror in 32 bit struct: UserDef (add 4 bytes)
Sizeerror in 64 bit struct: UserDef (add 4 bytes)

The check in question

    /* struct alignment */
    if (type >= firststruct) {
      if (sizeof(void *) == 8 && (size_native % 8)) {
        fprintf(stderr, "Align struct error: %s %s\n", types[structtype], cp);
        dna_error = 1;
      }
    }

Oh so little code, so many problems (and this patch will attempt to
fix just one of them, its only goals is to provide a helpful message
to the developer having issues, but feels like makesdna could use some
love)

No attempt is made to offer a solution, while one is clearly known,
kinda mean...
Makesdna is generally architecture agnostic, checks are being done
for both 64 bit and 32 bit. This one is protected by a
sizeof(void *) == 8 check. Which is a little fishy
If you do not know this code very well, (size_native % 8) feels
like this is about the size of the struct in question. You'd be wrong
though size_native is the offset in the parent structure where it
is used. A bit counter intuitive, so even if a dev goes "I don't know
what this means" and goes to lookup the error in makesdna, they are
likely to be mislead, kinda mean...
And then there's the alignment check itself which isn't inline with
what is actually required. The only reason this check works, it checks
for the worst case scenario (8), the actual alignment requirements are
"The largest native size in any of the sub structures" ie if an
embedded struct only has int16_t 's in it, there is no need to align
this to 8 bytes, 2 would have done just fine. We're being kinda
wasteful here.

Final score for 4 lines of code :

2x kinda mean
1x kinda fishy
1x kinda wasteful

We should be able to do better than this, no?

Future ?

makesdna is old code, we don't like touching it, and it's starting
to show its age a little bit, it's not very maintainable by
anyone except a few core devs that babysat it all its life. If
we have a list of work we know needs to be done, but that no-one
is going to volunteer for a rewrite of makesdna will be our shining
beacon on top of that list.

Give feedback on how many padding bytes are required and where to put them. ---- ## Heads up : The code change is really irrelevant It's a one liner, it's uncontroversial, it have comfortably landed without going though the review process. The patch description isn't very relevant to the problem it fixes but more of a general critique on the overall state of makesdna. target audience : The Core Module ## Background motivation Someone on chat asked for help , and makesdna was being "unhelpful" in my opion towards this dev, offering no useful suggestions how to resolve their problem. ## Repro put an `int oh_no_i_broke_things;` just before the `coba_weight` field in the `UesrDef` struct and you'll be met with the following "helpful" information. ``` Generating dna.c, dna_type_offsets.h, dna_verify.c CUSTOMBUILD : Align struct error : UserDef coba_weight CUSTOMBUILD : Align struct error : UserDef walk_navigation CUSTOMBUILD : Align struct error : UserDef space_data CUSTOMBUILD : Align struct error : UserDef file_space_data CUSTOMBUILD : Align struct error : UserDef experimental CUSTOMBUILD : Align struct error : UserDef runtime Sizeerror in 32 bit struct: UserDef (add 4 bytes) Sizeerror in 64 bit struct: UserDef (add 4 bytes) ``` ## The check in question ``` /* struct alignment */ if (type >= firststruct) { if (sizeof(void *) == 8 && (size_native % 8)) { fprintf(stderr, "Align struct error: %s %s\n", types[structtype], cp); dna_error = 1; } } ``` Oh so little code, so many problems (and this patch will attempt to fix just one of them, its only goals is to provide a helpful message to the developer having issues, but feels like makesdna could use some love) - No attempt is made to offer a solution, while one is clearly known, kinda mean... - Makesdna is generally architecture agnostic, checks are being done for both 64 bit and 32 bit. This one is protected by a `sizeof(void *) == 8` check. Which is a little fishy - If you do not know this code very well, `(size_native % 8)` feels like this is about the size of the struct in question. You'd be wrong though `size_native` is the offset in the parent structure where it is used. A bit counter intuitive, so even if a dev goes "I don't know what this means" and goes to lookup the error in makesdna, they are likely to be mislead, kinda mean... - And then there's the alignment check itself which isn't inline with what is actually required. The only reason this check works, it checks for the worst case scenario (8), the actual alignment requirements are "The largest native size in any of the sub structures" ie if an embedded struct only has `int16_t` 's in it, there is no need to align this to 8 bytes, 2 would have done just fine. We're being kinda wasteful here. Final score for 4 lines of code : 2x kinda mean 1x kinda fishy 1x kinda wasteful We should be able to do better than this, no? ## Future ? makesdna is old code, we don't like touching it, and it's starting to show its age a little bit, it's not very maintainable by anyone except a few core devs that babysat it all its life. If we have a list of work we know needs to be done, but that no-one is going to volunteer for a rewrite of makesdna will be our shining beacon on top of that list.

🎉 2

Ray molenkamp added 1 commit 2023-07-15 20:10:17 +02:00

84ad33425d makesdna: Give suggestions for fix

Give feedback on how many padding bytes are required and where to put
them.

----
## Heads up : The code change is really irrelevant

It's a one liner, it's uncontroversial, it have comfortably landed
without going though the review process. The patch description isn't
very relevant to the problem it fixes but more of a general critique
on the overall state of makesdna. target audience : The Core Module

## Background motivation

Someone on chat asked for help , and makesdna was being "unhelpful"
in my opion towards this dev, offering no useful suggestions how to
resolve their problem.

## Repro

put an `int oh_no_i_broke_things;` just before the `coba_weight` field
in the `UesrDef` struct and you'll be met with the following "helpful"
information.

```
Generating dna.c, dna_type_offsets.h, dna_verify.c
CUSTOMBUILD : Align struct error : UserDef coba_weight
CUSTOMBUILD : Align struct error : UserDef walk_navigation
CUSTOMBUILD : Align struct error : UserDef space_data
CUSTOMBUILD : Align struct error : UserDef file_space_data
CUSTOMBUILD : Align struct error : UserDef experimental
CUSTOMBUILD : Align struct error : UserDef runtime
Sizeerror in 32 bit struct: UserDef (add 4 bytes)
Sizeerror in 64 bit struct: UserDef (add 4 bytes)
```

## The check in question

```
    /* struct alignment */
    if (type >= firststruct) {
      if (sizeof(void *) == 8 && (size_native % 8)) {
        fprintf(stderr, "Align struct error: %s %s\n", types[structtype], cp);
        dna_error = 1;
      }
    }
```
Oh so little code, so many problems (and this patch will attempt to
fix just one of them, its only goals is to provide a helpful message
to the developer having issues, but feels like makesdna could use some
love)

- No attempt is made to offer a solution, while one is clearly known,
kinda mean...

- Makesdna is generally architecture agnostic, checks are being done
for both 64 bit and 32 bit. This one is protected by a
`sizeof(void *) == 8` check. Which is a little fishy

- If you do not know this code very well, `(size_native % 8)` feels
like this is about the size of the struct in question. You'd be wrong
though `size_native` is the offset in the parent structure where it
is used. A bit counter intuitive, so even if a dev goes "I don't know
what this means" and goes to lookup the error in makesdna, they are
likely to be mislead, kinda mean...

- And then there's the alignment check itself which isn't inline with
what is actually required. The only reason this check works, it checks
for the worst case scenario (8), the actual alignment requirements are
"The largest native size in any of the sub structures" ie if an
embedded struct only has `int16_t` 's in it, there is no need to align
this to 8 bytes, 2 would have done just fine. We're being kinda
wasteful here.

Final score for 4 lines of code :

2x kinda mean
1x kinda fishy
1x kinda wasteful

We should be able to do better than this, no?

## Future ?

makesdna is old code, we don't like touching it, and it's starting
to show its age a little bit, it's not very maintainable by
anyone except a few core devs that babysat it all its life. If
we have a list of work we know needs to be done, but that no-one
is going to volunteer for a rewrite of makesdna will be our shining
beacon on top of that list.

Ray molenkamp requested review from Campbell Barton 2023-07-15 20:10:49 +02:00

Ray molenkamp requested review from Sergey Sharybin 2023-07-15 20:11:02 +02:00

Sergey Sharybin approved these changes 2023-07-17 09:56:26 +02:00

Ray molenkamp referenced this pull request

2023-07-21 20:08:55 +02:00

Makesdna: more verbose `Align struct error` #110291

Sybren A. Stüvel commented

2023-07-24 12:26:51 +02:00

Interesting how we both sought to fix this at pretty much the same time. My PR is in #110291 and landed, but I agree that this PR is doing the better thing -- it's clearer for the developer who reads the message what to do about it.

Ray molenkamp closed this pull request

2023-07-25 16:26:11 +02:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations