Azure / azure-iot-sdk-c

A C99 SDK for connecting devices to Microsoft Azure IoT services
https://azure.github.io/azure-iot-sdk-c
Other
587 stars 739 forks source link

Serializer supports only 61 model attributes #281

Closed mredd closed 6 years ago

mredd commented 6 years ago

When updating from the 2015-11-30 pre-release to the current version, I discovered that our model is too big for the serializer. By current version, I refer to 2017-11-03, 2017-10-20, and 2016-10-14 (for testing purposes).

As of today, the model we're using has one WITH_ACTION item and 89 WITH_DATA items. It compiled fine with the 2015-11-30 release, but it crashes on the current version. I haven't figured out yet why this happens now and why it didn't happen in the 2015-11-30 release, because the COUNT_ARG implementation (which I think is the critical factor) seems to be the same.

However, this is a really annoying bug, because our model hasn't even grown to the final size yet and we would like to upgrade and stay on top of the development.

Description of the issue:

There are multiple issues. First, the INC(x) and DEC(x) macros are not implemented high enough. Right now, only implementations up to INC1024 and DEC1024 are available. Extend this up to i.e. INC1600 and DEC1600, respectively.

When all INC and DEC steps can be performed, the preprocessor runs into other errors like FOR_EACH_2_1 not found, which does not make sense, because FOR_EACH_2(...) shall only expand to even suffixes. My observation is, that the implementation of COUNT_ARG(...) reaches the limit of max. 127 parameters for a macro definition (refer to C standard, chapter 5.2.4.1). Some compilers allow for longer parameter lists, but they are not required. This might explain the slightly different behavior in clang and gcc.

Code sample exhibiting the issue:

#include <serializer.h>

#ifdef __cplusplus
extern "C" {
#endif

BEGIN_NAMESPACE(ns);

// This is a non-compiling model because it has too many attributes. Comment
// one arbitrary attribute to make this a compiling model.
DECLARE_MODEL(TestModel,
  WITH_DATA(int, attr1),
  WITH_DATA(int, attr2),
  WITH_DATA(int, attr3),
  WITH_DATA(int, attr4),
  WITH_DATA(int, attr5),
  WITH_DATA(int, attr6),
  WITH_DATA(int, attr7),
  WITH_DATA(int, attr8),
  WITH_DATA(int, attr9),
  WITH_DATA(int, attr10),
  WITH_DATA(int, attr11),
  WITH_DATA(int, attr12),
  WITH_DATA(int, attr13),
  WITH_DATA(int, attr14),
  WITH_DATA(int, attr15),
  WITH_DATA(int, attr16),
  WITH_DATA(int, attr17),
  WITH_DATA(int, attr18),
  WITH_DATA(int, attr19),
  WITH_DATA(int, attr20),
  WITH_DATA(int, attr21),
  WITH_DATA(int, attr22),
  WITH_DATA(int, attr23),
  WITH_DATA(int, attr24),
  WITH_DATA(int, attr25),
  WITH_DATA(int, attr26),
  WITH_DATA(int, attr27),
  WITH_DATA(int, attr28),
  WITH_DATA(int, attr29),
  WITH_DATA(int, attr30),
  WITH_DATA(int, attr31),
  WITH_DATA(int, attr32),
  WITH_DATA(int, attr33),
  WITH_DATA(int, attr34),
  WITH_DATA(int, attr35),
  WITH_DATA(int, attr36),
  WITH_DATA(int, attr37),
  WITH_DATA(int, attr38),
  WITH_DATA(int, attr39),
  WITH_DATA(int, attr40),
  WITH_DATA(int, attr41),
  WITH_DATA(int, attr42),
  WITH_DATA(int, attr43),
  WITH_DATA(int, attr44),
  WITH_DATA(int, attr45),
  WITH_DATA(int, attr46),
  WITH_DATA(int, attr47),
  WITH_DATA(int, attr48),
  WITH_DATA(int, attr49),
  WITH_DATA(int, attr50),
  WITH_DATA(int, attr51),
  WITH_DATA(int, attr52),
  WITH_DATA(int, attr53),
  WITH_DATA(int, attr54),
  WITH_DATA(int, attr55),
  WITH_DATA(int, attr56),
  WITH_DATA(int, attr57),
  WITH_DATA(int, attr58),
  WITH_DATA(int, attr59),
  WITH_DATA(int, attr60),
  WITH_DATA(int, attr61),
  WITH_DATA(int, attr62)
);

END_NAMESPACE(ns);

#ifdef __cplusplus
}
#endif

Console log of the issue:

Here is the compile output when compiling above source sample as iot-bug.c. The INC and DEC macros were already extended in macro_utils.h.

<source-dir>/iot-bug/iot-bug.c:11:1: warning: implicit declaration of function 'FOR_EACH_2_1' is invalid in C99 [-Wimplicit-function-declaration]
DECLARE_MODEL(TestModel,
^
<build-dir>/azure-iot-sdk-src/serializer/inc/serializer.h:177:5: note: expanded from macro 'DECLARE_MODEL'
    TO_AGENT_DATA_TYPE(name, DROP_FIRST_COMMA_FROM_ARGS(EXPAND_MODEL_ARGS(__VA_ARGS__)))     \
    ^
<build-dir>/azure-iot-sdk-src/serializer/inc/serializer.h:417:13: note: expanded from macro 'TO_AGENT_DATA_TYPE'
            FOR_EACH_2(FIELD_AS_STRING, EXPAND_TWICE(__VA_ARGS__)) \
            ^
<build-dir>/azure-iot-sdk-src/c-utility/inc/azure_c_shared_utility/macro_utils.h:8560:42: note: expanded from macro 'FOR_EACH_2'
#define FOR_EACH_2(MACRO_TO_INVOKE, ...) C2(FOR_EACH_2_, C1(COUNT_ARG(__VA_ARGS__))) ( MACRO_TO_INVOKE, __VA_ARGS__)
                                         ^
<build-dir>/azure-iot-sdk-src/c-utility/inc/azure_c_shared_utility/macro_utils.h:5509:17: note: expanded from macro 'C2'
#define C2(x,y) C2_(x,y)
                ^
<build-dir>/azure-iot-sdk-src/c-utility/inc/azure_c_shared_utility/macro_utils.h:5507:18: note: expanded from macro 'C2_'
#define C2_(x,y) x##y
                 ^
<scratch space>:52:1: note: expanded from here
FOR_EACH_2_1
^
<source-dir>/iot-bug/iot-bug.c:11:1: warning: implicit declaration of function 'FOR_EACH_1_COUNTED_COUNT_ARG_DISPTACH_EMPTY_intintintint' is invalid in C99 [-Wimplicit-function-declaration]
<build-dir>/azure-iot-sdk-src/serializer/inc/serializer.h:177:30: note: expanded from macro 'DECLARE_MODEL'
    TO_AGENT_DATA_TYPE(name, DROP_FIRST_COMMA_FROM_ARGS(EXPAND_MODEL_ARGS(__VA_ARGS__)))     \
                             ^
<build-dir>/azure-iot-sdk-src/serializer/inc/serializer.h:377:5: note: expanded from macro 'DROP_FIRST_COMMA_FROM_ARGS'
    FOR_EACH_1_COUNTED(DROP_IF_EMPTY, __VA_ARGS__)
    ^
<build-dir>/azure-iot-sdk-src/c-utility/inc/azure_c_shared_utility/macro_utils.h:8562:50: note: expanded from macro 'FOR_EACH_1_COUNTED'
#define FOR_EACH_1_COUNTED(MACRO_TO_INVOKE, ...) C2(FOR_EACH_1_COUNTED_, C1(COUNT_ARG(__VA_ARGS__))) ( MACRO_TO_INVOKE,  __VA_ARGS__)
                                                 ^
<build-dir>/azure-iot-sdk-src/c-utility/inc/azure_c_shared_utility/macro_utils.h:5509:17: note: expanded from macro 'C2'
#define C2(x,y) C2_(x,y)
                ^
<build-dir>/azure-iot-sdk-src/c-utility/inc/azure_c_shared_utility/macro_utils.h:5507:18: note: expanded from macro 'C2_'
#define C2_(x,y) x##y
                 ^
<scratch space>:21:1: note: expanded from here
FOR_EACH_1_COUNTED_COUNT_ARG_DISPTACH_EMPTY_intintintint
^
<source-dir>/iot-bug/iot-bug.c:11:1: error: expected expression
<build-dir>/azure-iot-sdk-src/serializer/inc/serializer.h:177:57: note: expanded from macro 'DECLARE_MODEL'
    TO_AGENT_DATA_TYPE(name, DROP_FIRST_COMMA_FROM_ARGS(EXPAND_MODEL_ARGS(__VA_ARGS__)))     \
                                                        ^
<build-dir>/azure-iot-sdk-src/serializer/inc/serializer.h:398:24: note: expanded from macro 'EXPAND_MODEL_ARGS'
    FOR_EACH_1_COUNTED(TO_AGENT_DT_EXPAND_ELEMENT_ARGS, __VA_ARGS__)
                       ^
<source-dir>/iot-bug/iot-bug.c:12:13: error: expected expression
  WITH_DATA(int, attr1),
            ^
<source-dir>/iot-bug/iot-bug.c:13:13: error: expected expression
  WITH_DATA(int, attr2),
            ^
<source-dir>/iot-bug/iot-bug.c:14:13: error: expected expression
  WITH_DATA(int, attr3),
            ^
<source-dir>/iot-bug/iot-bug.c:15:13: error: expected expression
  WITH_DATA(int, attr4),
            ^
<source-dir>/iot-bug/iot-bug.c:16:13: error: expected expression
  WITH_DATA(int, attr5),
            ^
<source-dir>/iot-bug/iot-bug.c:17:13: error: expected expression
  WITH_DATA(int, attr6),
            ^
<source-dir>/iot-bug/iot-bug.c:18:13: error: expected expression
  WITH_DATA(int, attr7),
            ^
<source-dir>/iot-bug/iot-bug.c:19:13: error: expected expression
  WITH_DATA(int, attr8),
            ^
<source-dir>/iot-bug/iot-bug.c:20:13: error: expected expression
  WITH_DATA(int, attr9),
            ^
<source-dir>/iot-bug/iot-bug.c:21:13: error: expected expression
  WITH_DATA(int, attr10),
            ^
<source-dir>/iot-bug/iot-bug.c:22:13: error: expected expression
  WITH_DATA(int, attr11),
            ^
<source-dir>/iot-bug/iot-bug.c:23:13: error: expected expression
  WITH_DATA(int, attr12),
            ^
<source-dir>/iot-bug/iot-bug.c:24:13: error: expected expression
  WITH_DATA(int, attr13),
            ^
<source-dir>/iot-bug/iot-bug.c:25:13: error: expected expression
  WITH_DATA(int, attr14),
            ^
<source-dir>/iot-bug/iot-bug.c:26:13: error: expected expression
  WITH_DATA(int, attr15),
            ^
<source-dir>/iot-bug/iot-bug.c:27:13: error: expected expression
  WITH_DATA(int, attr16),
            ^
<source-dir>/iot-bug/iot-bug.c:28:13: error: expected expression
  WITH_DATA(int, attr17),
            ^
<source-dir>/iot-bug/iot-bug.c:29:13: error: expected expression
  WITH_DATA(int, attr18),
            ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
2 warnings and 20 errors generated.
make[3]: *** [iot-bug/CMakeFiles/iot-bug.dir/iot-bug.c.o] Error 1
make[2]: *** [iot-bug/CMakeFiles/iot-bug.dir/all] Error 2
make[1]: *** [iot-bug/CMakeFiles/iot-bug.dir/rule] Error 2
make: *** [iot-bug] Error 2
jspaith commented 6 years ago

I'm able to reproduce this issue. Even though a fair amount has changed in the last 2 years internally, I apologize that you've had to bump into this.

My first question is whether you need to put 89 (and growing) parameters into a flat structure like this? I verified on gcc 5.4 / Ubuntu 16.04 that the below compiles:

DECLARE_MODEL(innerData1,
WITH_DATA(ascii_char_ptr, DeviceId1),
WITH_DATA(ascii_char_ptr, DeviceId2),
[many fields]
)

DECLARE_MODEL(innerData2,
WITH_DATA(ascii_char_ptr, DeviceId1),
[many fields]
)

DECLARE_MODEL(ContosoAnemometer,
WITH_DATA(ascii_char_ptr, DeviceId),
WITH_DATA(innerData1, inner_Data1),
WITH_DATA(innerData2, inner_Data2),
[many fields]
)

The sum of all the fields for my test was well over 100, we only hit issue on macro expansion on an individual model being > 61.

I understand you may have back-compat concerns with existing JSON, but separating this out would get you unblocked and going right away and not bumping into preprocessor limits.

The 127 limit was introduced because Visual Studio can't handle more than this in its preprocessor. I'm not that familiar with the internals (I can learn more as needed) but in c-utility\tools\macro_utils_h_generator\macro_utils.tt there's fields called nArithmetic and nMacroParameters which specify the maximums of INC/DEC and the arg counts, respectively. (We set 124 and not 127 because we need 1st 3 fields for our own book-keeping.) You should be able to rebuild this project with higher maximums.

But... if segmenting the fields into sub-structures works for your JSON, I'd strongly recommend trying that approach 1st.

mredd commented 6 years ago

Thank you for your quick response. Actually, each of the model attributes is a data structure. Modeling them as plain integers was just for the sake of demonstration. Our model looks more like this:

DECLARE_STRUCT(s1,
  ascii_char_ptr, attr_s1_1,
  int, attr_s1_2,
  int, attr_s1_3
);

DECLARE_STRUCT(s2,
  int64_t, attr_s2_1,
  EDM_DATE_TIME_OFFSET, attr_s2_2,
  int, attr_s2_3
);

// And way more structs

DECLARE_MODEL(TestModel,
  WITH_DATA(s1, attr1),
  WITH_DATA(s2, attr2),
  [many more fields]
);

So, overall, we are already pushing some hundred plain datatype attributes to the IoT endpoint.

What is the difference in using DECLARE_STRUCT vs. DECLARE_MODEL for the type definitions?

jspaith commented 6 years ago

I believe DECLARE_STRUCT is a simple C style struct, whereas DECLARE_MODEL gets you methods and also is how you interact with the IoTHub API when it comes time to serialize. Either path would get you into the preprocessor maximum though.

Regarding the TestModel() many fields, how much flexibility do you have on its layout? I think you could (in theory) do something instead of

DECLARE_MODEL(TestModel,
   field1 // of whatever type
....
    field1000
;

Instead do something like

DECLARE_MODEL(TestModel,
    Fields1-20 // And this is a struct containing fields 1-20, regardless of their type
    Fields21-40 // And with more meaningful names than 21-40:)
    [...].

This gets you out of preprocessor issues, but if you have pre-existing JSON out in field you may not have that level of flexibility.

mredd commented 6 years ago

This gets you out of preprocessor issues, but if you have pre-existing JSON out in field you may not have that level of flexibility.

We have devices in the upper five digits already deployed in the field. Plus quite some amount of code on the cloud side.

If we can find a workaround which produces compatible JSON, I'm all in. But changing the JSON layout and hence the database model behind the IoT endpoint is above my paygrade. That triggers a HUGE change process at the cloud team. We had that discussion a few times because we are still carrying some learning-curve flaws in our model. :-/

jspaith commented 6 years ago

I completely understand about changing JSON after its in production, but I wanted to ask anyway since I wasn't sure where you were at in deployment.

I'm doing some experiments on the branch for c-utilites 'serializer-params' (I know it shouldn't be called that as c-utils shouldn't know about serializer, but as I said experimental...). In it I've changed the .tt file to give more params, recompile, and see additional #def's but I'm getting same set of errors. I'm reaching out to some other folks for additional guidance and continuing research, but you can take a look also if you want as you've gone pretty deep here, too.

jspaith commented 6 years ago

I'm able to workaround the issue. Here's what you'll need to do:

I haven't attempted e2e tests, but this feels like one where once it compiles everything just flows naturally.

jspaith commented 6 years ago

@mredd as this has been dormant for about a week, I'm going to close just for cleanliness. If you have followups by all means re-activate this or open a new git hub issue.

mredd commented 6 years ago

This was telepathic. I'm testing it right now. Sorry for the delay, I was working on another topic this week. Expect my findings in about an hour.

mredd commented 6 years ago

The serializer-params branch works here as well. @jspaith Will you merge it into master?

jspaith commented 6 years ago

I'm afraid I can't merge this into master. All the options I can see for doing so are going to cause risk/complexity. I'll be transparent with reasoning and if I'm missing something that can help you out, I'm very interested to learn.

Our options would be: 1 - Just take directly to master. Obviously a no-go because of Visual Studio limitations. 2 - Create two macro files, the existing one (for Visual Studio) and this large one (gcc and others?). The danger here is that if a customer develops a solution on Linux using 100 parameters, they can't port to Windows. Cross-SDK portability is pretty important. 3 - We could create a CMAKE option like "allow-long-serializer-lists". Adding CMAKE first means customers would risk non-portability of (2), and also problematic is it expands our test matrix and complexity of product. We're trying to leave cmake flags for config like which TLS stack is used and minimize it generally, or else soon we'll have something unmaintainable over time.

While I'm happy we've got you a workaround, I apologize that I can't give you a better solution than having to fork.