Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix printf format string for int_32t #80

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Coderitter-GmbH
Copy link
Contributor

fixes #76

@alrevuelta
Copy link
Owner

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
Copy link
Contributor Author

Coderitter-GmbH commented Jan 8, 2021

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
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
Contributor Author

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

@nopeslide
Copy link
Collaborator

nopeslide commented Jan 11, 2021

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
Copy link
Collaborator

nopeslide commented Mar 3, 2021

@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
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
Copy link
Contributor Author

Coderitter-GmbH commented Mar 5, 2021

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