From df7ab47dec8c9c0e1c8560fa098adbf6f44abbd8 Mon Sep 17 00:00:00 2001 From: Ryan Blue Date: Tue, 5 Nov 2024 01:10:23 -0500 Subject: [PATCH] Fix dynamic struct decoding for nested structs After a struct-type field descriptor had offsets calculated more than once, IsBitField would return true, causing the second call to CalculateOffsets to calculate incorrect offsets. --- .../first/util/struct/StructDescriptor.java | 4 ++- .../util/struct/StructFieldDescriptor.java | 2 +- .../native/include/wpi/struct/DynamicStruct.h | 4 ++- .../first/util/struct/DynamicStructTest.java | 26 +++++++++++++++-- .../native/cpp/struct/DynamicStructTest.cpp | 28 ++++++++++++++++--- 5 files changed, 54 insertions(+), 10 deletions(-) diff --git a/wpiutil/src/main/java/edu/wpi/first/util/struct/StructDescriptor.java b/wpiutil/src/main/java/edu/wpi/first/util/struct/StructDescriptor.java index 438db434c4b..16d01328fe1 100644 --- a/wpiutil/src/main/java/edu/wpi/first/util/struct/StructDescriptor.java +++ b/wpiutil/src/main/java/edu/wpi/first/util/struct/StructDescriptor.java @@ -6,8 +6,10 @@ import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.Stack; /** Raw struct dynamic struct descriptor. */ @@ -148,7 +150,7 @@ void calculateOffsets(Stack stack) { private final String m_name; String m_schema; - final List m_references = new ArrayList<>(); + final Set m_references = new HashSet<>(); final List m_fields = new ArrayList<>(); final Map m_fieldsByName = new HashMap<>(); int m_size; diff --git a/wpiutil/src/main/java/edu/wpi/first/util/struct/StructFieldDescriptor.java b/wpiutil/src/main/java/edu/wpi/first/util/struct/StructFieldDescriptor.java index 1d5db379fdb..d6a1edaa57a 100644 --- a/wpiutil/src/main/java/edu/wpi/first/util/struct/StructFieldDescriptor.java +++ b/wpiutil/src/main/java/edu/wpi/first/util/struct/StructFieldDescriptor.java @@ -224,7 +224,7 @@ public long getIntMax() { * @return true if bitfield */ public boolean isBitField() { - return m_bitShift != 0 || m_bitWidth != (m_size * 8); + return (m_bitShift != 0 || m_bitWidth != (m_size * 8)) && m_struct == null; } private final StructDescriptor m_parent; diff --git a/wpiutil/src/main/native/include/wpi/struct/DynamicStruct.h b/wpiutil/src/main/native/include/wpi/struct/DynamicStruct.h index e807ab7e44c..36d323e625f 100644 --- a/wpiutil/src/main/native/include/wpi/struct/DynamicStruct.h +++ b/wpiutil/src/main/native/include/wpi/struct/DynamicStruct.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -234,7 +235,8 @@ class StructFieldDescriptor { * @return true if bitfield */ bool IsBitField() const { - return m_bitShift != 0 || m_bitWidth != (m_size * 8); + return (m_bitShift != 0 || m_bitWidth != (m_size * 8)) && + m_struct == nullptr; } private: diff --git a/wpiutil/src/test/java/edu/wpi/first/util/struct/DynamicStructTest.java b/wpiutil/src/test/java/edu/wpi/first/util/struct/DynamicStructTest.java index 78a8127f433..13745a90ba6 100644 --- a/wpiutil/src/test/java/edu/wpi/first/util/struct/DynamicStructTest.java +++ b/wpiutil/src/test/java/edu/wpi/first/util/struct/DynamicStructTest.java @@ -51,14 +51,34 @@ void testNestedStruct() { void testDelayedValid() { var desc = assertDoesNotThrow(() -> db.add("test", "foo a")); assertFalse(desc.isValid()); - var desc2 = assertDoesNotThrow(() -> db.add("test2", "foo a[2]")); + var desc2 = assertDoesNotThrow(() -> db.add("test2", "foo a; foo b;")); assertFalse(desc2.isValid()); - var desc3 = assertDoesNotThrow(() -> db.add("foo", "int32 a")); - assertTrue(desc3.isValid()); + var desc3 = assertDoesNotThrow(() -> db.add("test3", "foo a[2]")); + assertFalse(desc2.isValid()); + var desc4 = assertDoesNotThrow(() -> db.add("foo", "int32 a")); + assertTrue(desc4.isValid()); assertTrue(desc.isValid()); assertEquals(desc.getSize(), 4); assertTrue(desc2.isValid()); assertEquals(desc2.getSize(), 8); + assertTrue(desc3.isValid()); + assertEquals(desc3.getSize(), 8); + } + + @Test + void testReuseNestedStructDelayed() { + var desc2 = assertDoesNotThrow(() -> db.add("test2", "test a;test b;")); + var desc = assertDoesNotThrow(() -> db.add("test", "int32 a; uint16 b; int16 c;")); + assertTrue(desc.isValid()); + assertTrue(desc2.isValid()); + assertEquals(desc2.getSize(), 16); + var fields = desc2.getFields(); + var field = fields.get(0); + assertEquals(field.getOffset(), 0); + assertEquals(field.getName(), "a"); + field = fields.get(1); + assertEquals(field.getOffset(), 8); + assertEquals(field.getName(), "b"); } @Test diff --git a/wpiutil/src/test/native/cpp/struct/DynamicStructTest.cpp b/wpiutil/src/test/native/cpp/struct/DynamicStructTest.cpp index d326263a227..ac3fd6da54f 100644 --- a/wpiutil/src/test/native/cpp/struct/DynamicStructTest.cpp +++ b/wpiutil/src/test/native/cpp/struct/DynamicStructTest.cpp @@ -43,16 +43,36 @@ TEST_F(DynamicStructTest, DelayedValid) { auto desc = db.Add("test", "foo a", &err); ASSERT_TRUE(desc); ASSERT_FALSE(desc->IsValid()); - auto desc2 = db.Add("test2", "foo a[2]", &err); + auto desc2 = db.Add("test2", "foo a;foo b;", &err); ASSERT_TRUE(desc2); ASSERT_FALSE(desc2->IsValid()); - auto desc3 = db.Add("foo", "int32 a", &err); - ASSERT_TRUE(desc3); - ASSERT_TRUE(desc3->IsValid()); + auto desc3 = db.Add("test3", "foo a[2]", &err); + ASSERT_TRUE(desc2); + ASSERT_FALSE(desc2->IsValid()); + auto desc4 = db.Add("foo", "int32 a", &err); + ASSERT_TRUE(desc4); + ASSERT_TRUE(desc4->IsValid()); ASSERT_TRUE(desc->IsValid()); ASSERT_EQ(desc->GetSize(), 4u); ASSERT_TRUE(desc2->IsValid()); ASSERT_EQ(desc2->GetSize(), 8u); + ASSERT_TRUE(desc3->IsValid()); + ASSERT_EQ(desc3->GetSize(), 8u); +} + +TEST_F(DynamicStructTest, ReuseNestedStructDelayed) { + auto desc2 = db.Add("test2", "test a;test b;", &err); + auto desc = db.Add("test", "int32 a; uint16 b; int16 c;", &err); + ASSERT_TRUE(desc); + ASSERT_TRUE(desc->IsValid()); + ASSERT_TRUE(desc2); + ASSERT_TRUE(desc2->IsValid()); + ASSERT_EQ(desc2->GetSize(), 16u); + auto fields = desc2->GetFields(); + ASSERT_EQ(fields[0].GetOffset(), 0u); + ASSERT_EQ(fields[0].GetName(), "a"); + ASSERT_EQ(fields[1].GetOffset(), 8u); + ASSERT_EQ(fields[1].GetName(), "b"); } TEST_F(DynamicStructTest, InvalidBitfield) {