alrevuelta / cONNXr

Pure C ONNX runtime with zero dependancies for embedded devices
MIT License
193 stars 31 forks source link

Fix printf format string for int_32t #80

Open Coderitter-GmbH opened 3 years ago

Coderitter-GmbH commented 3 years ago

fixes #76

alrevuelta commented 3 years ago

Nice. Could you also check these other data_type occurrences? So that we always print int_32 types with the same string format? https://github.com/alrevuelta/cONNXr/blob/aceace00aa575fbc3512e5da1c53f9fe667f4548/include/test/test_utils.h https://github.com/alrevuelta/cONNXr/blob/aceace00aa575fbc3512e5da1c53f9fe667f4548/src/connxr.c https://github.com/alrevuelta/cONNXr/blob/aceace00aa575fbc3512e5da1c53f9fe667f4548/include/tracing.h

Just fixed the windows CI, should be working now if you rebase.

Coderitter-GmbH commented 3 years ago

I hope a merge is okay. Tracing uses a feature which is NOT C99 so I can't test any changes (any definition of trace level won't compile). You are right, there are other occurrences, i just never used the connxr.c file. I will look into it.

alrevuelta commented 3 years ago

"Tracing uses a feature which is NOT C99". Had no idea about this. Since we want to keep this as portable as possible and C99 compliant can @nopeslide have a look into it? Not strictly related to this PR so moving this discussion to #81. Let me both know what you think.

Coderitter-GmbH commented 3 years ago

Additional fixed test_utils.c and connxr.c. For tracing.h I need some help, it goes over my head.

nopeslide commented 3 years ago

Additional fixed test_utils.c and connxr.c. For tracing.h I need some help, it goes over my head.

should be quite the same. I could do it, but it will take a few days, since I have no spare time atm

nopeslide commented 3 years ago

@Coderitter-GmbH sorry for the delay

I looked into the tracing.h file and found these location where I mistyped:

diff --git a/include/tracing.h b/include/tracing.h
index f5fcd77a..f138ad19 100644
--- a/include/tracing.h
+++ b/include/tracing.h
 #define __PREAMBLE_ABORT(FD, LEVEL) \
@@ -651,10 +651,10 @@ if (COND) { \
     } \
     if (__TRACE_COND(LEVEL)) { \
         __VAR(LEVEL, "TENSOR", TENSOR->name, "             \"%s\"\n"); \
-        __VAR(LEVEL, "TENSOR", TENSOR->data_type, "        %d") \
+        __VAR(LEVEL, "TENSOR", TENSOR->data_type, "        %" PRId32) \
         if ((TENSOR->data_type) >= _n_tensor_types) { \
             __PRINT(TRACE_SYMBOL_STDOUT, " (%s)\n", _tensor_types[0]) \
-            __BOUND_ERROR(0, TENSOR->data_type, 0, _n_tensor_types, "%d") \
+            __BOUND_ERROR(0, TENSOR->data_type, 0, _n_tensor_types, "%" PRId32) \
             __ERROR(0, "unknown data type") \
         } else { \
             __PRINT(TRACE_SYMBOL_STDOUT, " (%s)\n", \

all other occurrences of %d refer to actual system dependent int declaration:

 $ grep -Hn '%d' include/tracing.h
include/tracing.h:671:        __VAR(LEVEL, "TENSOR", TENSOR->has_data_type, "    %d\n") \
include/tracing.h:682:        __VAR(LEVEL, "TENSOR", TENSOR->has_raw_data, "     %d\n") \
include/tracing.h:687:        __VAR(LEVEL, "TENSOR", TENSOR->has_data_location, "%d\n") \
include/tracing.h:688:        __VAR(LEVEL, "TENSOR", TENSOR->data_location, "    %d\n") \
include/tracing.h:705:        __VAR(LEVEL, "ATTRIBUTE", ATTR->type, "            %d") \
include/tracing.h:708:            __BOUND_ERROR(0, ATTR->type, 0, _n_attribute_types, "%d") \
include/tracing.h:715:        __VAR(LEVEL, "ATTRIBUTE", ATTR->has_type, "        %d\n") \
include/tracing.h:716:        __VAR(LEVEL, "ATTRIBUTE", ATTR->has_f, "           %d\n") \
include/tracing.h:718:        __VAR(LEVEL, "ATTRIBUTE", ATTR->has_i, "           %d\n") \
include/tracing.h:720:        __VAR(LEVEL, "ATTRIBUTE", ATTR->has_s, "           %d\n") \
include/tracing.h:893:        __VAR(LEVEL, "MODEL", MODEL->has_ir_version, "   %d\n") \
include/tracing.h:906:        __VAR(LEVEL, "MODEL", MODEL->has_model_version, "%d\n") \

see src/pb/protobuf-c.h for definitions of these: the has_* attributes are booleans that are defined as int

/** Boolean type. */
typedef int protobuf_c_boolean;

and the other attribute are enums with the size of an int:

#ifndef PROTOBUF_C__FORCE_ENUM_TO_BE_INT_SIZE
 #define PROTOBUF_C__FORCE_ENUM_TO_BE_INT_SIZE(enum_name) \
  , _##enum_name##_IS_INT_SIZE = INT_MAX
#endif

I have no idea what other side effects these system dependent definitions have. Afaik protobuf is a binary protocol and I'm a little bit confused with this boolean/enum definition (why not cast a value with fixed length into an enum?) Could you maybe check if my diff is enough? if not: please check these enums with a simple sizeof on your side

nopeslide commented 3 years ago

I have no idea what other side effects these system dependent definitions have. Afaik protobuf is a binary protocol and I'm a little bit confused with this boolean/enum definition

If I understood it correctly protobuf encodes numbers (regardless of type) with variable length, the resulting type is only a cast and therefore not system dependent.

Coderitter-GmbH commented 3 years ago

Could you maybe check if my diff is enough? if not: please check these enums with a simple sizeof on your side

I will look into it.