Skip to content

Fix printf format string for int_32t#80

Open
Coderitter-GmbH wants to merge 2 commits into
alrevuelta:masterfrom
Coderitter-GmbH:int32_t
Open

Fix printf format string for int_32t#80
Coderitter-GmbH wants to merge 2 commits into
alrevuelta:masterfrom
Coderitter-GmbH:int32_t

Conversation

@Coderitter-GmbH

Copy link
Copy Markdown
Contributor

fixes #76

@alrevuelta

Copy link
Copy Markdown
Owner

@Coderitter-GmbH

Coderitter-GmbH commented Jan 8, 2021

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Owner

"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

Copy link
Copy Markdown
Contributor Author

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

@nopeslide

nopeslide commented Jan 11, 2021

Copy link
Copy Markdown
Collaborator

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

nopeslide commented Mar 3, 2021

Copy link
Copy Markdown
Collaborator

@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

Copy link
Copy Markdown
Collaborator

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

Coderitter-GmbH commented Mar 5, 2021

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong printf format string for type int32_t

3 participants