Fix bug caused by uninitialized variable in typechecker

[mihaibudiu]

Signed-off-by: Mihai Budiu mbudiu@vmware.com
Fixes #3719
Also fixed a couple of warnings in gc.cpp
There is no simple test, but hopefully @smolkaj can confirm that the bug is fixed.

[smolkaj]

I tried manually and can confirm that it fixes the issue.

Thanks so much Mihai for the incredibly fast diagnosis & fix!

[smolkaj]

Any ideas why https://github.com/p4lang/p4c/blob/main/testdata/p4_16_samples/pins/pins_middleblock.p4 didn't catch this, @mbudiu-vmw ?

[jafingerhut]

@smolkaj I do not know why your tests were failing, but when I built with latest p4c source code, without the changes in this PR, using the following commands on an Ubuntu 20.04 system:

cd p4c
mkdir build
cd build
cmake .. -DCMAKE_BUILD_TYPE=DEBUG
make -j2

I got no errors or crashes at all when compiling the version of pins_middleblock.p4 that is in the p4c repo today, nor when attempting to compile the version of middleblock.p4 in this repo: https://github.com/p4lang/p4c/blob/main/testdata/p4_16_samples/pins/pins_middleblock.p4

I don't know why you did, but we could have been using different versions of OS, glibc, C++ compiler, etc. as I mentioned above. I can give you the versions I ran with, if it is of interest to you, but I don't know how else to explain it than whatever is different about our build and/or run-time environments.

[jafingerhut]

I can approve of this if you want, Mihai, but I don't know the code, so not sure why you would ask me for review unless it is that I respond relatively quickly :-)

[mihaibudiu]

Uninitialized variables are not deterministic. If it happens to be 0 all works well.

[smolkaj]

Makes sense (well, sort of, but this is C++ after all) :)

Andy, on the other thread we discussed that both Mihai and I were able to reproduce with Bazel, but not with cmake.
If we had latent undefined behavior here I suppose that's not surprising.

By the way, Mihai, props for getting your hands dirty with Bazel 👍 Thanks a lot for fixing this, once again!

[rst0git]

LGTM

cppcheck is also able to detect this problem:

$ cppcheck frontends/p4/typeChecking/typeChecker.cpp --inconclusive --enable=warning
Checking frontends/p4/typeChecking/typeChecker.cpp ...
frontends/p4/typeChecking/typeChecker.cpp:131:16: warning: Member variable 'TypeInference::currentActionList' is not initialized in the constructor. [uninitMemberVar]
TypeInference::TypeInference(ReferenceMap* refMap, TypeMap* typeMap, bool readOnly,
               ^

[mihaibudiu]

@fruffy this fails with the following report: testgen-p4c-bmv2/issue3702-bmv2.p4
Is this a true positive?
Even if it is a true positive, I assume it is not really related to this PR.
If it is a true positive, can you please file a reproduction for it?
I will merge this PR anyway, since it fixes a real bug.

[fruffy]

This is an issue that has already been fixed.

[davidbolvansky]

Uninitialized variables are not deterministic. If it happens to be 0 all works well.

If think we could enhance our Clang CI build with additional flag -ftrivial-auto-var-init=pattern - I can try.
https://clang.llvm.org/docs/ClangCommandLineReference.html

See it in action:
https://godbolt.org/z/fPMdf3Gsx

Real bugs would be more exposed now.

[apinski-cavium]

If think we could enhance our Clang CI build with additional flag -ftrivial-auto-var-init=pattern - I can try.

If you are using GCC 12 or above, you can use the same option too.

[davidbolvansky]

If think we could enhance our Clang CI build with additional flag -ftrivial-auto-var-init=pattern - I can try.

If you are using GCC 12 or above, you can use the same option too.

Yeah :)