Paths

Table of Contentst

WIP: Fix to RNA function ParamList alignment issue
AbandonedPublic
Actions

Authored by Loren Osborn (linux_dr) on Apr 28 2022, 11:08 PM.

Details

Reviewers

Summary

When the RNA function dispatch system builds a ParameterList, it doesn't currently respect type alignment. This is a fix for this issue described in T97691

I found this issue trying to clear out preexisting warnings from blender to help diagnose issues in another subsystem. It was caught by ~~Xcode's~~ Undefined Behavior Sanitizer, which I expect isn't commonly used by most developers ~~as it is a macOS exclusive IDE~~. (@Ray Molenkamp (LazyDodo) pointed out that this is a cross-platform tool, and requires the code to be custom compiled to support it, so it isn't purely runtime analysis.)

This is a proof of concept/work-in-progress implementation. It simply adds 0 or more padding bytes to the size of the previous parameter. (ALERT possible memory violation needing fix:) This likely has issues copying from/to off the end of the parameters being padded. While this could be a serious issue, the easy solution is to make the ParameterList aware of the size and padding of elements individually, and should not be a difficult addition to the code.

This is a more polished PR but still will probably benefit from advice from more seasoned Blender developers.

Alternative solutions might include modify the code to work with actual structs for each ParameterList so that the compiler can manage the field alignments, but I suspect that would be a much bigger code change. We could also use std::function<> templates to expose the function arguments differently, but this would likely be even more work. I think the std::function<> is likely the best long term solution using modern C++ semantics, but the purpose of this patch is to fix the issue without additional growing pains.

Aside from the memory violation issue sighted above, I think it's possible that an array parameter could need inter-element padding. This is also a simple addition, but I don't see any of the types currently supported needing it. (This would be required if a type's size is smaller than, or not a multiple of its alignment.) (I was unaware that the language hides this complexity from developers, and because we mostly only pass basic types here, the type size and alignment are usually the same.)

No external change is proposed, so no mockup is included.

Diff Detail

Repository: rB Blender

Event Timeline

Loren Osborn (linux_dr) requested review of this revision.Apr 28 2022, 11:08 PM

Loren Osborn (linux_dr) created this revision.

Loren Osborn (linux_dr) added inline comments.Apr 28 2022, 11:22 PM

.gitignore
47 ↗	(On Diff #50976)	I added this, as I thought it was helpful, but it doesn't belong in this patch.
build_files/cmake/project_info.py
173 ↗	(On Diff #50976)	Spelling fix: doesn't belong in this patch.
source/blender/makesrna/intern/makesrna.c
3000	I keep seeing `parm` and reading "parmesan"... May we rename this `param`?
3004	I really think this should be: fprintf(f, "\t_data += sizeof(%s)", parm_typename); if (parm_padding > 0) { fprintf(f, " + %d", parm_padding); } fprintf(f, ";\n"); but we'd need the type name as a string, and the padding as a separate value.
source/blender/makesrna/intern/rna_define.c
4375	I think `rna_parameter_size()` and `rna_parameter_alignment()` should be one function that populates a struct instead.
4434	This seems like it overlaps some with `ParameterIterator` which probably needs some changes to handle size and padding as distinct.

Loren Osborn (linux_dr) added inline comments.Apr 28 2022, 11:35 PM

source/blender/makesrna/intern/rna_access.c
7058	THIS IS THE SOURCE OF THE POSSIBLE MEMORY CORRUPTION: This `size` can later be used for `memcpy()`s which can read off the end of memory, or worse, write over the end of memory.
7175	(memory corruption)...and here
7187	(memory corruption)...and here

Hi Mr. Fijas,

Ray mentioned ideasman42 was PPR of the RNA system. I was kinda crossing my fingers guessing that was you. I hope you find this helpful.

Thanks,

-Loren

Loren Osborn (linux_dr) edited reviewers, added: Eric Park (ideaman924); removed: Ideas Fijas (ideasfijas).Apr 28 2022, 11:49 PM

Loren Osborn (linux_dr) mentioned this in T97693: [Mac:Xcode] Mysterious pointer-alignment requirement.Apr 29 2022, 12:07 AM

Loren Osborn (linux_dr) edited reviewers, added: Campbell Barton (campbellbarton), Bastien Montagne (mont29), Brecht Van Lommel (brecht); removed: Eric Park (ideaman924).Apr 29 2022, 12:11 AM

@Jesse Yurkovich (deadpin), sent me a link to the subsystem maintainers, so I linked them to this patch. -Thank you.

better not add a whole bunch of reviewers to a diff that's not even ready

Brecht Van Lommel (brecht) requested changes to this revision.Apr 29 2022, 1:55 AM

Brecht Van Lommel (brecht) added a subscriber: Brecht Van Lommel (brecht).

Brecht Van Lommel (brecht) added inline comments.

source/blender/makesrna/intern/rna_access.c
7042–7050	`MEM_mallocN_aligned` should be unnecessary, we're not using that for every struct allocation that may contain arbitrary types, and this is no different as far as I can tell.
7175	I don't think changing the size here makes sense. It should be modifying only the offset.
7187	Same comment.
source/blender/makesrna/intern/rna_define.c
4375	I don't think we need a `rna_parameter_alignment` function, we can use `power_of_2_max_i(rna_parameter_size(parm))`. These parameters are small enough that it doesn't matter if there is some wasted space.
4434	I think this concept of "aligned parameter size" is confusing. We should be thinking in terms of aligning the offset, not the size. I think a function like this would be better: int rna_parameter_align_offset(const int offset, PropertyRNA *parm) { const int alignment = power_of_2_max_i(rna_parameter_size(parm)); return (offset + alignment - 1) & ~(alignment - 1); }

This revision now requires changes to proceed.Apr 29 2022, 1:55 AM

Loren Osborn (linux_dr) added inline comments.Jun 6 2022, 4:39 PM

source/blender/makesrna/intern/rna_access.c
7042–7050	Since then I learned that all `malloc` Allocations default to `alignof(std::max_align_t)` (which is intended to be `sizeof(void*)`), which means explicitly aligned allocations should only be necessary for weird types used for things like SSE instructions. I will update this in my next revision

Loren Osborn (linux_dr) added inline comments.Jun 20 2022, 10:03 PM

source/blender/makesrna/intern/rna_define.c
4375	There are places where the alignment isn't the same as the type size, for instance: `sizeof(ParameterDynAlloc)`
4434	I think I adopted the intent of this approach, even if I haven't done it the same way.

Fairly complete overhaul based on feedback and changes I hadn't made time for with my proof of concept. padding and alignment are now maintained as distinct values from size. Additionally, I'm using string representations of the types to be included in the generated code that now differentiates "padding" bytes from data bytes and which types we're computing sizeof() from.

Loren Osborn (linux_dr) edited the summary of this revision. (Show Details)Jun 20 2022, 10:16 PM

Loren Osborn (linux_dr) added a subscriber: Ray Molenkamp (LazyDodo).

Loren Osborn (linux_dr) marked 4 inline comments as done and an inline comment as not done.Jun 20 2022, 10:40 PM

As I mentioned in #blender-coders, my biggest outstanding question is where should I put the defines I currently have in source/blender/makesrna/intern/rna_internal.h? Thank you in advance for your help.

Thanks for the work. But I was thinking you could basically just do this in a few places, with the function I suggested.

int offset = 0;
for (parm = func->cont.properties.first; parm; parm = parm->next) {
  offset = rna_parameter_align_offset(offset, parm);
  void *data = ((char*)data_start) + offset;
  ...
  offset += rna_parameter_size(parm);
}

Or alternatively something like this if that's an easier fit:

int offset = 0;
for (parm = func->cont.properties.first; parm; parm = parm->next) {
  void *data = ((char*)data_start) + offset;

  offset += rna_parameter_size_with_padding(parm, parm->next);
}

It may need to be tweaked a bit for different places, but I don't understand why we need all the additional complexity in this patch. I understand that a simple approach may overestimate the alignment requirements, but I think that's fine and the additional complexity of trying to fit things more tightly is not worth it.

This revision now requires changes to proceed.Jun 21 2022, 7:02 PM

I think just this should be enough to fix the ubsan warning. It's likely faster overall due to the simpler logic, and memory usage is not a concern.

P3025 (An Untitled Masterwork)

1	diff --git a/source/blender/makesrna/intern/makesrna.c b/source/blender/makesrna/intern/makesrna.c
2	index 400944d..b535451 100644
3	--- a/source/blender/makesrna/intern/makesrna.c
4	+++ b/source/blender/makesrna/intern/makesrna.c
5	@@ -3030,7 +3030,7 @@ static void rna_def_function_funcs(FILE f, StructDefRNA dsrna, FunctionDefRNA
6	}
7
8	if (dparm->next) {
9	- fprintf(f, "\t_data += %d;\n", rna_parameter_size(dparm->prop));
10	+ fprintf(f, "\t_data += %d;\n", rna_parameter_size_pad(rna_parameter_size(dparm->prop)));
11	}
12	}
13
14	diff --git a/source/blender/makesrna/intern/rna_access.c b/source/blender/makesrna/intern/rna_access.c
15	index 0bc35d8..a0b25cf 100644
16	--- a/source/blender/makesrna/intern/rna_access.c
17	+++ b/source/blender/makesrna/intern/rna_access.c
18	@@ -7130,7 +7130,7 @@ ParameterList RNA_parameter_list_create(ParameterList parms,
19
20	/* allocate data */
21	for (parm = func->cont.properties.first; parm; parm = parm->next) {
22	- alloc_size += rna_parameter_size(parm);
23	+ alloc_size += rna_parameter_size_pad(rna_parameter_size(parm));
24
25	if (parm->flag_parameter & PARM_OUTPUT) {
26	parms->ret_count++;
27	@@ -7206,7 +7206,7 @@ ParameterList RNA_parameter_list_create(ParameterList parms,
28	}
29	}
30
31	- data = ((char *)data) + rna_parameter_size(parm);
32	+ data = ((char *)data) + rna_parameter_size_pad(size);
33	}
34
35	return parms;
36	@@ -7230,7 +7230,7 @@ void RNA_parameter_list_free(ParameterList *parms)
37	}
38	}
39
40	- tot += rna_parameter_size(parm);
41	+ tot += rna_parameter_size_pad(rna_parameter_size(parm));
42	}
43
44	MEM_freeN(parms->data);
45	@@ -7272,7 +7272,7 @@ void RNA_parameter_list_begin(ParameterList parms, ParameterIterator iter)
46
47	void RNA_parameter_list_next(ParameterIterator *iter)
48	{
49	- iter->offset += iter->size;
50	+ iter->offset += rna_parameter_size_pad(iter->size);
51	iter->parm = iter->parm->next;
52	iter->valid = iter->parm != NULL;
53
54	diff --git a/source/blender/makesrna/intern/rna_define.c b/source/blender/makesrna/intern/rna_define.c
55	index 9d26797..5363d4e 100644
56	--- a/source/blender/makesrna/intern/rna_define.c
57	+++ b/source/blender/makesrna/intern/rna_define.c
58	@@ -4421,6 +4421,16 @@ int rna_parameter_size(PropertyRNA *parm)
59	return sizeof(void *);
60	}
61
62	+int rna_parameter_size_pad(const int size)
63	+{
64	+ /* Pad parameters in memory so the next parameter is properly aligned.
65	+ * This silences warnings in ubsan. More complicated logic to pack parameters
66	+ * more tightly in memory is unlikely to improve performance, and aligning
67	+ * to the requirements for pointers is enough for all data types we use. */
68	+ const int alignment = sizeof(void *);
69	+ return (size + alignment - 1) & ~(alignment - 1);
70	+}
71	+
72	/* Dynamic Enums */
73
74	void RNA_enum_item_add(EnumPropertyItem *items, int totitem, const EnumPropertyItem *item)
75	diff --git a/source/blender/makesrna/intern/rna_internal.h b/source/blender/makesrna/intern/rna_internal.h
76	index 9e743a4..6ca8e66 100644
77	--- a/source/blender/makesrna/intern/rna_internal.h
78	+++ b/source/blender/makesrna/intern/rna_internal.h
79	@@ -635,6 +635,7 @@ PointerRNA rna_pointer_inherit_refine(struct PointerRNA ptr, struct StructRNA
80	/* Functions */
81
82	int rna_parameter_size(struct PropertyRNA *parm);
83	+int rna_parameter_size_pad(const int size);
84
85	/* XXX, these should not need to be defined here~! */
86	struct MTex rna_mtex_texture_slots_add(struct ID self,

Jesse Yurkovich (deadpin) removed a subscriber: Jesse Yurkovich (deadpin).Jun 22 2022, 7:29 PM

@Brecht Van Lommel (brecht),

This is a barebones, stripped down version of my RNA alignment patch. We may choose that https://developer.blender.org/P3025 is the simpler fix, and I see no problem going with that, but the changes here are much more clear to me. (Of course, I'm biased, as I have an easier time understanding changes I wrote myself.)

P3025 is a smaller delta, and that may ultimately make it less complex, but maybe some bits of this delta are easier to understand.

Perhaps P3025 will make more sense to me when I don't have a 9 year old tugging on my arm, begging me to play Pokemon with him.

I look forward to your feedback.

-Loren

Thanks for the effort in making this patch, but I still prefer to go with the simpler solution.

There's also a performance impact here since some of this code is run for every RNA function call, and for me that's decisive in going with the simpler solution.

Brecht Van Lommel (brecht) mentioned this in rBdc64673f6e09: Fix T97691: undefined behavior sanitizer warning for alignment in RNA functions.Jun 23 2022, 7:23 PM

I have one or two more ideas on this. I do think more static asserts (that have no runtime impact) would be helpful. I may or may not be able you be able to do more conservative packing without additional overhead. Thank you for your patience with my experimentation.

source/blender/makesrna/intern/makesrna.c
3004	Would improving the readability of the generated code be acceptable as a separate patch?0

I would rather not spend more time on this, I don't think we need more asserts or readability in generated code.

In D14798#412719, @Brecht Van Lommel (brecht) wrote:

I would rather not spend more time on this, I don't think we need more asserts or readability in generated code.

I completed both diffs before I saw you reply. I may change my mind in the future, but I agree for now, to not waste brain cells on them.

(One diff has zero runtime impact, and purely documents and enforces our assumptions, that may be platform specific. I may submit this one later if I still feel it’s important.)

Thank you for your generous investment of time and attention with this issue. I hope I can contribute more substantive changes in the future.

Loren Osborn (linux_dr) mentioned this in T99382: [Parent Task] Fix UBSan issues.Jul 4 2022, 9:30 AM