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.
rzblue · Nov 5, 2024 · df7ab47 · df7ab47
1 parent debb521
commit df7ab47
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 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
@@ -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<StructDescriptor> stack) {
 
   private final String m_name;
   String m_schema;
-  final List<StructDescriptor> m_references = new ArrayList<>();
+  final Set<StructDescriptor> m_references = new HashSet<>();
   final List<StructFieldDescriptor> m_fields = new ArrayList<>();
   final Map<String, StructFieldDescriptor> 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
@@ -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
@@ -10,6 +10,7 @@
 #include <span>
 #include <string>
 #include <string_view>
+#include <unordered_set>
 #include <utility>
 #include <vector>
 
@@ -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
@@ -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
@@ -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) {