Issues and odd things found via clang warnings

[mtfurlan]

Hello! I'm using SAE J2735 201603, and putting the output from ASN1C through clang with -Weverything. While going through and disabling specific warnings, I saw what might be bugs, or at least things that can be improved. There is stuff like -Wconditional-uninitialized that I couldn't easily follow the code enough to know if it's an issue, I'm just going to list stuff I can understand or at least kinda follow.

I'm going to link to output examples in an unrelated repo that has output for J2735 from an unknown version of ASN1c, though the output I reference matches the output from the current `vlm_master branch. (dcf963c0e43196057a97feac16421fe79dc7d943)

The full list of warnings I ended up with is:

-Wall \
-Wextra \
-Weverything \
-Werror \
-Wno-missing-field-initializers \
-Wno-unused-parameter \
-Wno-parentheses-equality \
-Wno-switch-enum \
-Wno-conversion \
-Wno-switch-default \
-Wno-reserved-id-macro \
-Wno-shorten-64-to-32 \ 
-Wno-gnu-empty-struct \ 
-Wno-c++-compat \ 
-Wno-cast-align \ 
-Wno-used-but-marked-unused \ 
-Wno-assign-enum \ 
-Wno-conditional-uninitialized \
-Wno-unreachable-code-break \
-Wno-covered-switch-default \
-Wno-comma \
-Wno-unreachable-code \
-Wno-unused-macros \
-Wno-padded

Going to call out the ones I think can be easily fixed or I understand well enough to say they look suspicious.

• -Wunreachable-code:
  • constr_CHOICE_print.c:37
  • uper_opentype.c:219
  • What is the reason for these existing? If they should just be removed I can do that.
• -Wused-but-marked-unused:
  • In a few places like things are marked unused with CC_NOTUSED, but are then used.
  • example
  • I can try to make a minmal test case if anyone who wants to look into this doesn't have J2735.
• -Wstring-conversion: A few places convert a string to a bool like INTEGER.c:657
  • It looks like this is just an assert that will always fail and giving a reason why, but I feel like assert(0) //Unreachable would be better, I can do a PR for this.
• -Wshorten-64-to-32 -Wconversion: Several places do stuff like
  • assign an int32_t into an int8_t
  • size_t into int
• -Wcomma: A bunch of things in asn_bit_data.c use commas instead of semicolons, such as line 171
  • I don't think there is any benefit to using commas instead of semicolons here? I can create a PR to change this if there isn't any reason for this to stay as commas.
• -Wparentheses-equality
  • example
  • This is just kinda odd?

Should I make PRs to clean up the items I have mentioned above? If you can give guidance on how the code generation works I may be able to make PRs for the other things.

[mouse07410]

-Wunreachable-code: constr_CHOICE_print.c:37 uper_opentype.c:219

What is the reason for these existing?

If memory serves, some enhancements caused conflict, that these were the workarounds. I do not recall the details, so want the "evidence" of what it looked like earlier, to stay there. Maybe I'll make these "less offensive" to the compiler, but I don't want to completely obliterate the "trace". And it's not a bug. So, for now it stays as-is.

-Wused-but-marked-unused: In a few places like things are marked unused with CC_NOTUSED, but are then used.

I did not see (or you did not show) where it was marked CC_NOTUSED. Please show more details.

-Wstring-conversion: A few places convert a string to a bool like INTEGER.c:657

That is not "string-to-bool". It's parsing an integer, returning success or graduated failure, and (as I understand) to pacify the compiler, adds an assert before return that should never happen. As it's not a bug, let's leave it alone.

-Wshorten-64-to-32 -Wconversion: Several places do stuff like "assign an int32_t into an int8_t" or "size_t into int"

Re. int32_t -> int8_t: the code ensures that only 7 bits of the int32_t are used. How do you propose to change it? No PR, just a couple of lines of code to illustrate your point.

Re. size_t -> int: I sort of see your point - how do you propose to change the code here, considering that int value is used in many places in this function (so I doubt changing it to, e.g., ssize_t value would be a good idea)?

This probably is not a bug.

-Wcomma: A bunch of things in asn_bit_data.c use commas instead of semicolons...

Yes it does - because the order of those operations is unimportant, and it allows the compiler to re-arrange or run them in parallel. Definitey not a bug - therefore, this code stays as-is.

-Wparentheses-equality: example. This is just kinda odd?

It sure is, but I don't recognize this code example, and it comes from somebody else's repo - how did you get to it? And it doesn't really bother me. ;-)

Should I make PRs to clean up the items I have mentioned above?

I'd say - not at this time. If you could explain how you propose to change what's there in places I mentioned above - we can decide what to do with that code.

If you can give guidance on how the code generation works I may be able to make PRs for the other things.

I don't think I'm qualified to guide on the code generation here, which is one of the reasons I'm reluctant to "beautify" the code - there are plenty of dragons around, and I'm not itching to wake them up.

[mtfurlan]

For the parentheses equality example I have it in the output from J2735, but my company isn't ready to publish everything yet, and that repo has nearly the same output from j2735 via asn1c, so I used that.

For the integer conversion stuff I don't have a proposal, it just seems pretty suspicious.

For the unused thing,

clang -ggdb -DASN_PDU_COLLECTION -Wall -Wextra -Werror -Wno-missing-field-initializers -Wno-unused-parameter -Wno-switch-enum -Wno-conversion -Wno-switch-default -Weverything -Wno-shorten-64-to-32 -Wno-gnu-empty-struct -Wno-c++-compat -Wno-cast-align -Wno-assign-enum -Wno-conditional-uninitialized -Wno-unreachable-code-break -Wno-covered-switch-default -Wno-comma -Wno-unreachable-code -Wno-unused-macros -Wno-parentheses-equality -Wno-padded -Wno-reserved-id-macro -Wno-keyword-macro  -I./lib  -I./lib/j2735 -o build/obj/native/./lib/j2735/VehicleID.o -c lib/j2735/VehicleID.c
lib/j2735/VehicleID.c:80:4: error: 'asn_PER_type_VehicleID_constr_1' was marked unused but was used [-Werror,-Wused-but-marked-unused]
                &asn_PER_type_VehicleID_constr_1,
                 ^
1 error generated.
make: *** [Makefile:84: build/obj/native/./lib/j2735/VehicleID.o] Error 1

My actual VehicleID.c

I'm happy not changing anything, I opened this issue because I know that issues like #71 existed, so I suspect there may be other hiding edge cases, I just don't know the codebase well enough to differentiate suspicious conversion from actual bug.

[mouse07410]

For the parentheses equality example I have it in the output from J2735, but my company isn't ready to publish everything yet, and that repo has nearly the same output from j2735 via asn1c, so I used that...

I personally have no clue about J2735 - except that the authors seem to really abuse ASN.1 language, with predictable consequences.

Even if you do publish that code - I doubt I'd be able to make heads or tails of it... :-(

For the integer conversion stuff I don't have a proposal, it just seems pretty suspicious

The code is rather convoluted in some places, which is a counter-incentive (for me) to start changing it without an overwhelming reason.

Some of those assignments do smell, others are ugly but fine. Regardless, there's not enough horsepower to re-implement it... :-(

lib/j2735/VehicleID.c:80:4: error: 'asn_PER_type_VehicleID_constr_1' was marked unused but was used [-Werror,-Wused-but-marked-unused] &asn_PER_type_VehicleID_constr_1,

Was marked "unused" where? Who/what placed that "unused" mark?

My actual VehicleID.c

My firewall blocks access to that file (due to its location). Regardless, how did that file come to existence? Who wrote it? If it's auto-generated - from what ASN.1 source?

I'm happy not changing anything, I opened this issue because I know that issues like #71 existed, so I suspect there may be other hiding edge cases, I just don't know the codebase well enough to differentiate suspicious conversion from actual bug

Even #71 was not a bug per se - just an unnecessary limitation that we (you) were able to alleviate (in this case).

One "edge case" is size of constraints. So far we weren't able (not enough resources) to address it. The correct way would be to represent those constraints with INTEGER_t. Now they're intmax_t or so...

I don't know the codebase well enough either. There's about 150,000 lines of code (with comments). Major refactoring is beyond my capability and available time. :-(

[mtfurlan]

Oh I'm really sorry I edited out where I referred to the line it was defined. It's generated by asn1c from J2735. In the example repo it's defined here (I suspect the GCC_NOTUSED is what's marking it as unused) and used here. The line numbers don't quite match up with my compile output and there are some other changes which is why I tried to share my version.

The ASN1 for that is

VehicleID ::= CHOICE {
   entityID     TemporaryID,
   stationID    StationID
   }

I can't say I'm terribly surprised J2735 is an abuse of ASN.1, I've tried to follow pieces of it and just not understood many of the decisions.

Not enough time is always an issue... I'll see if I can convince $work to have me spend some time looking at the constraints thing, but I keep being told to worry about "deadlines" or something.

[mouse07410]

In the example repo it's defined here (I suspect the GCC_NOTUSED is what's marking it as unused) and used here.

I still don't understand who/what slapped the GCC_NOTUSED there. Do you think it's asn1c during code generation? There's no single occurrence of the text GCC_NOTUSED in the entire asn1c source base, so I seriously doubt it's asn1c output.

I can't say I'm terribly surprised J2735 is an abuse of ASN.1, I've tried to follow pieces of it and just not understood many of the decisions

Those decisions weren't driven by wisdom - so it isn't worth the effort trying to understand them. :-)

I'll see if I can convince $work to have me spend some time looking at the constraints thing, but I keep being told to worry about "deadlines" or something

I hear you. Similar situation - plus, what I'm doing really is more interesting... And I don't quite expect to find an actual use case when an INTEGER constraint doesn't fit into long long. I know some people do have those truly weird requirements - I just don't expect to find myself among them. :D

[mtfurlan]

Well It might help if I report the actual string it uses, it has CC_NOTUSED not GCC_NOTUSED. Definition comes from here: https://github.com/mouse07410/asn1c/blob/vlm_master/libasn1compiler/asn1c_C.c#L2242..L2246 Usage comes from here: https://github.com/mouse07410/asn1c/blob/vlm_master/libasn1compiler/asn1c_C.c#L3158

[mouse07410]

Well It might help if I report the actual string it uses . . .

It might indeed. :-)

It's trivial to remove - but I can't determine all the consequences of such a change, and a lot of the current validation tests rely on CC_NOTUSED being present. Also, I don't quite understand why it's only for UPER/APER...

We need to compare that code fragment to what's in the upstream master. I checked - same code is in the upstream master. Somehow, asn1c fails to detect that the thing is referenced.

[mtfurlan]

I'll try to look into the size of constraints at some point, but I think we've decided to not worry about any of the other stuff so I'm closing this issue.