lcm-proj / lcm

Lightweight Communications and Marshalling
GNU Lesser General Public License v2.1
980 stars 388 forks source link

lcmgen: Avoid java errors on byte arrays with int64_t lengths #356

Closed jwnimmer-tri closed 3 years ago

jwnimmer-tri commented 3 years ago

Closes #104.

ashuang commented 3 years ago

Can you modify so that the checks and casts only happen when the length is int64_t? seems like unnecessary work on smaller length types.

jwnimmer-tri commented 3 years ago

When I first looked at that, it wasn't immediately obvious how I'd look up the type of the data_size sibling field, while I'm in the middle of emitting the data field. I'll go look a bit harder, but it seemed like the code complexity within lcmgen would grow.

I decided not to bother with it in my initial attempt here because I trusted javac would take care of dead-code-elimination on a statically-false condition. Maybe I'll decompile the bytecode to see if that ended up being true.

I could also just remove the IOException checks entirely, if we don't think they're super helpful? All I really need are the (int) casts to silence the compiler, and we already have a bunch of those casts scattered around in the generated code without any overflow checks.

jwnimmer-tri commented 3 years ago

Maybe I'll decompile the bytecode to see if that ended up being true.

Wow, okay, javac doesn't recognize it as dead code -- it was emitting bytecode for the check! I'll see how difficult it is to short-circuit during lcmgen.

jwnimmer-tri commented 3 years ago

Seems like my options are either to

(1) cast long to int without any extra range checking;

(2) add a new data field to lcm_dimension_t that indicates whether the LCM_VAR refers to an oversize (64-bit) field name or not, populate it during parsing, and have emit_java guard the casts iff the variable is oversize.

Any votes?

jwnimmer-tri commented 3 years ago

I just pushed (2). If you'd prefer the simpler change of (1), I'm happy to prune it back.

ashuang commented 3 years ago

Thanks, @jwnimmer-tri . Yeah, I think we can just go with the simpler version of just casting to int.