json: Improve tolerance to uncovered C members during JSON encoding #53333

lucasdietrich · 2022-12-24T12:10:37Z

This enhancement fixes encoding of JSON objects for which descriptors do not covers all members of the C structure represented.

Fix #50976

Signed-off-by: Lucas Dietrich [email protected]

MaureenHelm · 2023-02-09T16:15:36Z

@mrfuchs can you please take a look?

carlescufi · 2023-03-02T16:20:27Z

@mrfuchs could you please take a look? Thanks!

MaureenHelm · 2023-03-30T15:08:26Z

@M1cha can you take a look?

M1cha · 2023-03-30T15:54:10Z

tests/lib/json/src/main.c

+struct obj_array_array obj_array_array_ts;
+
 ZTEST(lib_json_test, test_json_decoding_array_array)
 {
 	int ret;
-	struct obj_array_array obj_array_array_ts;


this change doesn't look necessary.

Indeed, reverted that

M1cha · 2023-03-30T16:00:32Z

include/zephyr/data/json.h

@@ -137,6 +138,9 @@ typedef int (*json_append_bytes_t)(const char *bytes, size_t len,
 				 __alignof__(type) == 2 ? 1 : \
 				 __alignof__(type) == 4 ? 2 : 3)

+/* Helper to get the size of a member within a struct. */
+#define Z_MEMBER_SIZE(type, member) sizeof(((type *)0)->member)


Given how often I needed this in other zephyr projects I'd say it's finally time to add this to include/zephyr/sys/util.h with the name SIZEOF_FIELD (that's what the linux kernel calls this, just lowercase).

M1cha · 2023-03-30T16:04:59Z

include/zephyr/data/json.h

-			size_t sub_descr_len;
+			uint16_t sub_descr_len;
+			uint16_t sub_struct_size;


what's the reason for the type change and why did you choose uint16_t?

I made the change to add a new field "sub_struct_size" without increasing the size of the structure
I made the hypothesis that a 16-bit unsigned integer would be sufficient to represent the values needed for these fields.
Should I use two size_t instead ?

I'd prefer not to touch sub_descr_len and thus keep it as size_t.

However, I'm wondering if this (unnamed) union is even the right place to add the struct size as it is primarily used to describe the descriptor (not the actual object). Couldn't we move the struct size outside of the union?

Reverted the type for sub_descr_len to size_t.

I do agree, I moved the field sub_struct_size up and renamed it to struct_size. It just required a little more work to have all descriptors defined correctly: I needed to explicitly define the struct_size member to an arbitrary value (e.g. 0).

carlescufi · 2023-04-07T11:15:27Z

@mrfuchs please take a look

mrfuchs · 2023-04-11T09:56:40Z

include/zephyr/data/json.h

-			size_t sub_descr_len;
+			uint16_t sub_descr_len;
+			uint16_t sub_struct_size;


I'd prefer not to touch sub_descr_len and thus keep it as size_t.

However, I'm wondering if this (unnamed) union is even the right place to add the struct size as it is primarily used to describe the descriptor (not the actual object). Couldn't we move the struct size outside of the union?

mrfuchs · 2023-04-11T10:04:34Z

lib/os/json.c

+	} else {
+		field = (char *)field + elem_descr->offset;


When the field pointer is updated and skipped over a (unused?) field, shouldn't the last_elem variable also be updated?

In this new design get_elem_size() returns the actual size of the underlying object, so last_elem correctly reflects the address of the last element.

However I'm still not convinced the way field is adjusted when parsing the nested array (when parsing an array of array).

However I'm still not convinced the way field is adjusted when parsing the nested array (when parsing an array of array).

Indeed. I hope that'll be fixed soon by #50816.

include/zephyr/sys/util.h

lucasdietrich · 2023-04-12T18:49:51Z

Did several modifications, but I still need to do additional testing.

github-actions · 2023-06-12T00:31:49Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

pdgendt · 2023-06-22T09:16:11Z

The align_shift member becomes obsolete if the struct size is to be used.

include/zephyr/data/json.h

include/zephyr/sys/util.h

tests/lib/json/src/main.c

pdgendt · 2023-06-27T09:07:36Z

@lucasdietrich care to update the PR for the latest comments, thanks

The tests in my PR #59485 succeed using this one.

lucasdietrich · 2023-06-27T09:49:09Z

Thank you for your review, I will make the changes this evening.

lucasdietrich · 2023-06-27T18:00:25Z

I believe I have addressed all your remarks @mrfuchs @pdgendt.

pdgendt · 2023-06-30T06:24:28Z

@lucasdietrich #50816 has been merged, please rebase and resolve conflicts, thanks!

lucasdietrich · 2023-06-30T11:02:10Z

Ok, i will rebase this evening.

lucasdietrich · 2023-07-03T06:36:51Z

Unfortunately I didn't have time to work on it yet, moreover the merge seems to be tricky.
I will need few more days.

pdgendt · 2023-07-25T06:42:34Z

@lucasdietrich any update on this?

lucasdietrich · 2023-07-25T22:14:11Z

After more investigation, I managed to get almost everything working except for the test "test_json_decoding_array_array", which has become a blocker for me.

I've somewhat lost track of the issue, but I can redirect my attention to it by tomorrow evening.

However, I might require some help to resolve it completely.

lucasdietrich · 2023-07-26T17:26:33Z

I managed to get almost everything working; we don't "guess" anymore the size of a structure but we compute it at build time and we use this to determine offsets.

The most tricky part was (and still is) the handling of nested arrays (array of arrays), which requires dedicated handling I'm not able to fully explain. To be honest I tried to have the unit tests working rather than designing an accurate solution for nested arrays. This is where I would greatly benefit from external insight. Consequently this has left the following key issue unresolved :

Yet, the member of the structure describing the nested array should remain at the start of the structure. Using the "test_json_encoding_array_array" test as example: I'm not yet able to calculate the correct offset of "objects" within the "struct array". Consequently the test fails (crashes) if we uncomment the line. Actually I don't fully understand when the adjustment should be made.

To conclude, I've successfully tested these commits with a personnal project.

lucasdietrich · 2023-07-26T18:16:07Z

I've just rebased and noticed that the new tests introduced in #60046 are failing with the current changes(2 dim array encoding). I'll need more time to address this issue.

This macro is used to get the size of a specific field in a structure type. It will return the size of the field in bytes. Signed-off-by: Lucas Dietrich <[email protected]>

Enable the computation of a struct member's size during compile-time to enhance accuracy, removing the need for speculative size determinations during runtime. Signed-off-by: Lucas Dietrich <[email protected]>

Add special handling for nested arrays This enhancement fix encoding of JSON objects for which descriptors do not covers all members of the C structure represented. Fixes zephyrproject-rtos#50976 Signed-off-by: Lucas Dietrich <[email protected]>

This commit updates various structs in the JSON test suite to include unused members. These additions aim to test the improved tolerance of JSON encoding for C struct members that may not be fully described by JSON descriptors. As a consequence of the addition of unused fields, prefer designated initializers instead of postionnal ones. Add a note about a test which constantly fails with a unused field. Signed-off-by: Lucas Dietrich <[email protected]>

github-actions · 2023-09-25T00:29:57Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

lucasdietrich requested review from nashif, dcpleung, andyross, carlescufi, galak and MaureenHelm as code owners December 24, 2022 12:10

zephyrbot added area: Base OS Base OS Library (lib/os) area: JSON labels Dec 24, 2022

zephyrbot requested a review from Sir-Branch December 24, 2022 12:11

zephyrbot assigned andyross Dec 24, 2022

MaureenHelm unassigned andyross Feb 9, 2023

carlescufi requested a review from mrfuchs March 2, 2023 16:20

M1cha reviewed Mar 30, 2023

View reviewed changes

lucasdietrich force-pushed the json_c_members_tolerance branch from 058d937 to 5b486cf Compare April 3, 2023 19:13

zephyrbot assigned andyross Apr 3, 2023

lucasdietrich force-pushed the json_c_members_tolerance branch from 5b486cf to de6b69c Compare April 3, 2023 19:17

carlescufi requested a review from M1cha April 7, 2023 11:15

mrfuchs requested changes Apr 11, 2023

View reviewed changes

lucasdietrich force-pushed the json_c_members_tolerance branch from de6b69c to 4520699 Compare April 12, 2023 18:45

github-actions bot added the Stale label Jun 12, 2023

pdgendt mentioned this pull request Jun 22, 2023

JSON incorrect object alignment in arrays with 64bit members #59585

Closed

pdgendt removed the Stale label Jun 22, 2023

pdgendt reviewed Jun 22, 2023

View reviewed changes

include/zephyr/data/json.h Show resolved Hide resolved

include/zephyr/sys/util.h Show resolved Hide resolved

tests/lib/json/src/main.c Show resolved Hide resolved

lucasdietrich force-pushed the json_c_members_tolerance branch 2 times, most recently from 16b2f34 to d37204c Compare June 27, 2023 17:30

lucasdietrich force-pushed the json_c_members_tolerance branch from d37204c to 1008bf9 Compare July 26, 2023 17:26

lucasdietrich force-pushed the json_c_members_tolerance branch 2 times, most recently from cf38367 to 5b088a1 Compare July 26, 2023 17:40

lucasdietrich added 4 commits July 26, 2023 21:27

sys: Introduce SIZEOF_FIELD helper macro in util.h

0c3bb8f

This macro is used to get the size of a specific field in a structure type. It will return the size of the field in bytes. Signed-off-by: Lucas Dietrich <[email protected]>

json: Introduce struct_size descriptor member

e3963a3

Enable the computation of a struct member's size during compile-time to enhance accuracy, removing the need for speculative size determinations during runtime. Signed-off-by: Lucas Dietrich <[email protected]>

github-actions bot added the Stale label Sep 25, 2023

github-actions bot closed this Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

json: Improve tolerance to uncovered C members during JSON encoding #53333

json: Improve tolerance to uncovered C members during JSON encoding #53333

lucasdietrich commented Dec 24, 2022

MaureenHelm commented Feb 9, 2023

carlescufi commented Mar 2, 2023

MaureenHelm commented Mar 30, 2023

M1cha Mar 30, 2023

lucasdietrich Apr 3, 2023

M1cha Mar 30, 2023

M1cha Mar 30, 2023

lucasdietrich Apr 3, 2023

mrfuchs Apr 11, 2023

lucasdietrich Apr 12, 2023

carlescufi commented Apr 7, 2023

mrfuchs Apr 11, 2023

mrfuchs Apr 11, 2023

lucasdietrich Apr 12, 2023

mrfuchs Jun 27, 2023

lucasdietrich commented Apr 12, 2023

github-actions bot commented Jun 12, 2023

pdgendt commented Jun 22, 2023

pdgendt commented Jun 27, 2023

lucasdietrich commented Jun 27, 2023

lucasdietrich commented Jun 27, 2023

pdgendt commented Jun 30, 2023

lucasdietrich commented Jun 30, 2023

lucasdietrich commented Jul 3, 2023

pdgendt commented Jul 25, 2023

lucasdietrich commented Jul 25, 2023

lucasdietrich commented Jul 26, 2023 •

edited

Loading

lucasdietrich commented Jul 26, 2023

github-actions bot commented Sep 25, 2023

json: Improve tolerance to uncovered C members during JSON encoding #53333

json: Improve tolerance to uncovered C members during JSON encoding #53333

Conversation

lucasdietrich commented Dec 24, 2022

MaureenHelm commented Feb 9, 2023

carlescufi commented Mar 2, 2023

MaureenHelm commented Mar 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi commented Apr 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasdietrich commented Apr 12, 2023

github-actions bot commented Jun 12, 2023

pdgendt commented Jun 22, 2023

pdgendt commented Jun 27, 2023

lucasdietrich commented Jun 27, 2023

lucasdietrich commented Jun 27, 2023

pdgendt commented Jun 30, 2023

lucasdietrich commented Jun 30, 2023

lucasdietrich commented Jul 3, 2023

pdgendt commented Jul 25, 2023

lucasdietrich commented Jul 25, 2023

lucasdietrich commented Jul 26, 2023 • edited Loading

lucasdietrich commented Jul 26, 2023

github-actions bot commented Sep 25, 2023

lucasdietrich commented Jul 26, 2023 •

edited

Loading