Fix issues with uint64 enums #5265

vglavnyy · 2019-03-26T15:12:44Z

Refactoring of EnumDef and EnumVal classes (#5161)

fix enum with uint64
add support of empty enums
hide the implementation of enums from code generators

The main idea: disable direct access to an enum value.

The consumers of enums are code generators. They need only string presentations of enums.
This PR hides implementation details of enums and delegates all operation with EnumVal to the owners (EnumDef). Added the internal builder of enums which hides the implementation of enums from the Parser.

All available tests passed.
New tests will be added to this PR before landing to cover most of the changes.

aardappel

Wow, this is a ton of cleanup, making the code generators more maintainable, and new tests too, thanks!

My comments are minor, and the CI failure is a flake, so I think we can merge as is.. you think it is ready?

aardappel · 2019-03-28T16:54:22Z

include/flatbuffers/idl.h

  Offset<reflection::EnumVal> Serialize(FlatBufferBuilder *builder, const Parser &parser) const;

  bool Deserialize(const Parser &parser, const reflection::EnumVal *val);

+  uint64_t GetAsUInt64() const { return static_cast<uint64_t>(value); }
+  int64_t GetAsInt64() const { return value; }
+  bool IsZero() const { return 0 == value; }


These two methods are nicely descriptive, but a bit superfluous, as using an int as a bool is fine in C++

IsZero and IsNotZero were added for visibility during refactoring.
The 'value` became private and I left these methods.

aardappel · 2019-03-28T16:56:16Z

include/flatbuffers/idl.h

  Offset<reflection::EnumVal> Serialize(FlatBufferBuilder *builder, const Parser &parser) const;

  bool Deserialize(const Parser &parser, const reflection::EnumVal *val);

+  uint64_t GetAsUInt64() const { return static_cast<uint64_t>(value); }


While I appreciate that this refactor is cleaner, we also have to think about backwards compatibility, since this class is a public API. In that way, I'd prefer for value to keep existing for the signed case, and there to be a method to get the unsigned one?

I guess it is no big deal for people that access value to replace it with your new method.

EnumVal and EnumDef are internal public. They are not used outside in the external (generated) code.

vglavnyy · 2019-03-30T01:22:16Z

Until Monday, I will add new tests to cover the changes. If all will pass we can try merge to master.

aardappel · 2019-04-01T19:06:45Z

Ok, let me know.

vglavnyy · 2019-04-03T17:52:46Z

@aardappel
Probably, cpp-parts of the EnumDef (parser and code generator) are ready.
Can you review these commits before squashing?
After C++, all other code generators should be updated and tested (at least with monster_enum.fbs).

I propose to limit the enums with bit_flags attribute to unsigned data types.

enum E: int8 (bit_flags) { s = 7 };
table T { y : E = s }

The binary value of s is -128 = 0xFFFFFFFFFFFFFF80LL (int64_t).
ANY value of this enum is also negative (see monster_enum_generated.h).
Checking that flag Fx is the member of ANY isn't easy after cast to signed or unsigned integer.
This cast may be implicit by promotion to int type.

auto r = cast<T*>(GetRoot());
int32_t i32 = static_cast<int32_t>(r->y()); // get default
uint32_t i32 = static_cast<uint32_t>(r->y());
i32 == -128; // FFFFFF80
u32 == 4294967168; // FFFFFF80
i32 & (1<<11) == true
u32 & (1<<11) == true

This is unexpected, even with experience in C++.

aardappel · 2019-04-05T19:00:16Z

Yes, likely constraining bit_flags to unsigned shouldn't break too many people. Or we could make it a warning for now.

aardappel

Wow this is a crazy amount of changes, hard for me to review what impact this has. But I can also see it would have been hard to split up. I think we're just going to have to merge it and see the impact.

I was thinking of doing a 1.11 release very soon. It may be wise to merge this right after, to reduce risk. On the other hand, merging it before has the advantage that the improvements will get to more people. WDYT?

src/idl_parser.cpp

vglavnyy · 2019-04-06T01:24:37Z

Better will be after 1.11.

I will try to extract all independent and test-passed changes to standalone requests.
After this, will do rebase to the master.
Constrain bit_flags underlying type to unsigned.
After the time, I will be able to review my code.

I caught memory leakage in the HEAD of the master trying to extract extended proto enum from this PR:

/// Enum doc comment.
enum ProtoEnum {
  option allow_alias = true;
  FOO = 1;
  /// Enum 2nd value doc comment misaligned.
  BAR = 5;
  // Aliases
  FOO_A1 = 1;
  BAR_A1 = 5;
  FOO_A2 = 1;
}

Is this enum declaration valid from point of view of Protobuf?

flatbuffers/tests/prototest/test.proto

Lines 8 to 14 in 94cb1ff

    
           /// Enum doc comment. 
        
           enum ProtoEnum { 
        
             FOO = 1; 
        
           /// Enum 2nd value doc comment misaligned. 
        
             BAR = 5; 
        
           }

Protobuf enum decl

There must be a zero value, so that we can use 0 as a numeric default value.

The zero value needs to be the first element, for compatibility with the proto2 semantics where the first enum value is always the default.

aardappel · 2019-04-08T17:08:32Z

What was the memory leak? What was leaking?

And yes, sounds like our test does not conform to Protobuf. Not sure if that is a problem, if FlatBuffers is more lenient than Protobuf we don't necessarily care to enforce ProtoBuf rules. In fact, most of the --proto parser is written under the assumption that the .proto is already correct according to protoc, if you feed it illegal protos it likely will give unhelpful errors or silently allow it.

aardappel · 2019-04-08T17:09:01Z

And yes, I will get started with the 1.11 release process right then.

vglavnyy · 2019-04-09T16:12:16Z

What was the memory leak? What was leaking?

GCC8.2, GCC7.3, Clang 7.0, MSVC2017 - detect leakage if proto-enum has aliased values.
I have checked Travis-CI does not detect it.

#0 0x6460c2 in operator new(unsigned long) (/home/osboxes/flatbuffers/.project/flattests+0x6460c2)
#1 0x69de90 in flatbuffers::Parser::ParseEnum(bool, flatbuffers::EnumDef**) /home/osboxes/flatbuffers/src/idl_parser.cpp:1668:19
#2 0x6b17f6 in flatbuffers::Parser::ParseProtoDecl() /home/osboxes/flatbuffers/src/idl_parser.cpp:2030:5
#3 0x6c4dc7 in flatbuffers::Parser::DoParse(char const*, char const**, char const*, char const*) /home/osboxes/flatbuffers/src/idl_parser.cpp:2584:7
#4 0x6c4a54 in flatbuffers::Parser::DoParse(char const*, char const**, char const*, char const*) /home/osboxes/flatbuffers/src/idl_parser.cpp:2573:16
#5 0x6beeb7 in flatbuffers::Parser::ParseRoot(char const*, char const**, char const*) /home/osboxes/flatbuffers/src/idl_parser.cpp:2420:3
#6 0x6975a3 in flatbuffers::Parser::Parse(char const*, char const**, char const*) /home/osboxes/flatbuffers/src/idl_parser.cpp:2401:13
#7 0x923d7a in ParseProtoTest() /home/osboxes/flatbuffers/tests/test.cpp:927:3
#8 0x942739 in FlatBufferTests() /home/osboxes/flatbuffers/tests/test.cpp:2637:5
#9 0x942de3 in main /home/osboxes/flatbuffers/tests/test.cpp:2695:3

If I right, the problem is located here:

flatbuffers/src/idl_parser.cpp

Lines 2033 to 2042 in 23bb574

    
           auto &v = enum_def->vals.vec; 
        
           std::sort(v.begin(), v.end(), compareEnumVals); 
        
           // Temp: remove any duplicates, as .fbs files can't handle them. 
        
           for (auto it = v.begin(); it != v.end();) { 
        
             if (it != v.begin() && it[0]->value == it[-1]->value) 
        
               it = v.erase(it); 
        
             else 
        
               ++it; 
        
           }

There are two issues:

memory leak: SymbolTable::vec.erase() does not call the destructor of EnumVal
invalid pointers in the SymbolTable::dict

The current PR fix both issues.

aardappel · 2019-04-11T17:14:08Z

Ok, but we said we weren't going to merge this one until after 1.11, so do you want to make a small PR that just fixes the leak (& invalid pointers) ?

vglavnyy · 2019-04-11T17:17:06Z

Ok, I'll prepare the patch.

- update the bit_flags enum monster_test::Color to unsigned type - declare the explicit size of generated enum names

aardappel · 2019-04-24T19:38:52Z

Ok, 1.11 landed, so this can now go in. Can you rebase?

- hide the implementation of enums from code generators - fix uint64 the issue in the cpp-generator - fix google#5108 - new tests - enums with bit_flags attribute should be unsigned

vglavnyy · 2019-04-27T16:05:53Z

@aardappel
Ready for review.

aardappel · 2019-04-29T16:49:00Z

src/idl_parser.cpp

+  FLATBUFFERS_CHECKED_ERROR AssignEnumeratorValue(const std::string &value) {
+    user_value = true;
+    auto ascending = false;
+    if (enum_def.IsUInt64()) {


There's a lot of code in this file that goes along this pattern of if (enum_def.IsUInt64()) followed by two very similar bits of code. I guess you evaluated that it wasn't worth templatizing?

Also, not necessarily for this PR, but worth a thought for the future: maybe we should start splitting up idl_parser.cpp into multiple files?

I thought about IsUInt64() generalization. Probably, this is impossible by design.
The IsUInt64() can't be used for a template specialization at compile time.
The EnumDef is the switch-based strategy to get access to enum values. This strategy emulates a virtual table for methods like ToString.
I have made IsUInt64() private to isolate from a high-level code.
It is possible to apply classical inheritance if it is needed.

About splitting idp_parser.cpp I thought too.
In time, a solution will be found.

include/flatbuffers/idl.h

- move EnumDef::ReverseLookup implementation to idl_parser.cpp - fix typos

vglavnyy · 2019-05-01T05:59:16Z

Ready for landing if two latest commits are ok.

aardappel · 2019-05-02T20:57:54Z

Yes, looks good.. thanks for your hard work, again :)

aardappel reviewed Mar 28, 2019

View reviewed changes

aardappel reviewed Apr 5, 2019

View reviewed changes

src/idl_parser.cpp Outdated Show resolved Hide resolved

vglavnyy added a commit to vglavnyy/flatbuffers that referenced this pull request Apr 6, 2019

Part of google#5265, neutral changes

b7e9d67

vglavnyy mentioned this pull request Apr 6, 2019

Part of 'uint64 enum fix PR', neutral changes #5281

Merged

aardappel pushed a commit that referenced this pull request Apr 8, 2019

Part of #5265, neutral changes (#5281)

dd6daa7

vglavnyy mentioned this pull request Apr 9, 2019

CI test with memory leak #5283

Merged

vglavnyy force-pushed the enum_u64_support branch 2 times, most recently from 42df4d4 to 7fa3327 Compare April 10, 2019 00:21

vglavnyy mentioned this pull request Apr 11, 2019

Cannot have vector of enum values for enums that do not contain 0 [C++, HEAD] #5284

Closed

vglavnyy force-pushed the enum_u64_support branch 5 times, most recently from 530a63c to 061da18 Compare April 12, 2019 17:26

vglavnyy added a commit to vglavnyy/flatbuffers that referenced this pull request Apr 12, 2019

Update generated code (part of google#5265)

7c773bf

- update the bit_flags enum monster_test::Color to unsigned type - declare the explicit size of generated enum names

vglavnyy mentioned this pull request Apr 27, 2019

[Rust] Too many enum values leads to compile error #5267

Closed

vglavnyy force-pushed the enum_u64_support branch 3 times, most recently from 0aab2d3 to 513e0bd Compare April 27, 2019 10:02

Fix issues with uint64 enums

89c6eee

- hide the implementation of enums from code generators - fix uint64 the issue in the cpp-generator - fix google#5108 - new tests - enums with bit_flags attribute should be unsigned

vglavnyy force-pushed the enum_u64_support branch from 513e0bd to 89c6eee Compare April 27, 2019 15:42

aardappel reviewed Apr 29, 2019

View reviewed changes

include/flatbuffers/idl.h Outdated Show resolved Hide resolved

aardappel approved these changes Apr 29, 2019

View reviewed changes

vglavnyy added 2 commits May 1, 2019 10:59

Refine objectives of EnumDef's FindByValue and ReverseLookup methods

f1f6081

- move EnumDef::ReverseLookup implementation to idl_parser.cpp - fix typos

Make the IsUInt64 method private

5823102

aardappel merged commit b8ef8c1 into google:master May 2, 2019

vglavnyy deleted the enum_u64_support branch June 2, 2019 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with uint64 enums #5265

Fix issues with uint64 enums #5265

vglavnyy commented Mar 26, 2019

aardappel left a comment

aardappel Mar 28, 2019

vglavnyy Mar 30, 2019

aardappel Mar 28, 2019

aardappel Mar 28, 2019

vglavnyy Mar 30, 2019

vglavnyy commented Mar 30, 2019

aardappel commented Apr 1, 2019

vglavnyy commented Apr 3, 2019

aardappel commented Apr 5, 2019 •

edited

Loading

aardappel left a comment

vglavnyy commented Apr 6, 2019

aardappel commented Apr 8, 2019

aardappel commented Apr 8, 2019

vglavnyy commented Apr 9, 2019

aardappel commented Apr 11, 2019

vglavnyy commented Apr 11, 2019

aardappel commented Apr 24, 2019

vglavnyy commented Apr 27, 2019

aardappel Apr 29, 2019

vglavnyy May 1, 2019

vglavnyy commented May 1, 2019

aardappel commented May 2, 2019

Fix issues with uint64 enums #5265

Fix issues with uint64 enums #5265

Conversation

vglavnyy commented Mar 26, 2019

aardappel left a comment

Choose a reason for hiding this comment

aardappel Mar 28, 2019

Choose a reason for hiding this comment

vglavnyy Mar 30, 2019

Choose a reason for hiding this comment

aardappel Mar 28, 2019

Choose a reason for hiding this comment

aardappel Mar 28, 2019

Choose a reason for hiding this comment

vglavnyy Mar 30, 2019

Choose a reason for hiding this comment

vglavnyy commented Mar 30, 2019

aardappel commented Apr 1, 2019

vglavnyy commented Apr 3, 2019

aardappel commented Apr 5, 2019 • edited Loading

aardappel left a comment

Choose a reason for hiding this comment

vglavnyy commented Apr 6, 2019

aardappel commented Apr 8, 2019

aardappel commented Apr 8, 2019

vglavnyy commented Apr 9, 2019

aardappel commented Apr 11, 2019

vglavnyy commented Apr 11, 2019

aardappel commented Apr 24, 2019

vglavnyy commented Apr 27, 2019

aardappel Apr 29, 2019

Choose a reason for hiding this comment

vglavnyy May 1, 2019

Choose a reason for hiding this comment

vglavnyy commented May 1, 2019

aardappel commented May 2, 2019

aardappel commented Apr 5, 2019 •

edited

Loading