search

NASA-SW-VnV / ikos

Static analyzer for C/C++ based on the theory of Abstract Interpretation.
Other
2.35k stars 171 forks source link

Fix for #273 #274

Closed Mechazo11 closed 1 month ago

Mechazo11 commented 2 months ago

Issue #273 stemmed from missing the #include <cstdint> in the liveness.hpp header file. This caused a build error both in Version 3.3 and 3.2. The fix is based on the suggestion by @ivanperez-keera.

ivanperez-keera commented 2 months ago

Thanks!

I know this is a small patch, but can you please sign one of the two CLAs: https://github.com/NASA-SW-VnV/ikos/tree/master/doc/contribute. Send it by email to ikos@lists.nasa.gov, CC-ing me ivan.perezdominguez@nasa.gov.

I hate to do this for such a small change, but I have to, especially because we are in the process of changing licenses (to one that will be more permissive).

Mechazo11 commented 2 months ago

@ivanperez-keera sure, I will do that. This is the first time I have contributed to such a major project and might not have understood how to contribute.

ivanperez-keera commented 2 months ago

No problem at all. And thanks for being accommodating! :)

Mechazo11 commented 2 months ago

@ivanperez-keera done. Let me know if there is anything else required from my end.

ivanperez-keera commented 2 months ago

I got your email. I'll review it quickly and merge this! Thanks!

asimonov commented 2 months ago

@Mechazo11, thanks for reporting this.

I was trying to upgrade space-ros to jazzy and discovered the same problems (https://github.com/space-ros/space-ros/issues/157#issuecomment-2305328933).

But in case of jazzy/ubuntu24 we need to do the same change in 3 files:

    modified:   analyzer/include/ikos/analyzer/analysis/liveness.hpp
    modified:   core/include/ikos/core/domain/machine_int/operator.hpp
    modified:   core/include/ikos/core/semantic/indexable.hpp

would it be possible for you to fix the other 2 files as you aleady have PR open?

greatly appreciated!

Mechazo11 commented 2 months ago

@asimonov thanks for the suggestion.

Dr. @ivanperez-keera, I haven't done this before so sorry asking but can I modify operator.hpp and indexable.hpp without needing to sign another CLA?

ivanperez-keera commented 2 months ago

Yes, you can. You don't need to sign another CLA. You can add it to the same PR. Thanks!

Mechazo11 commented 2 months ago

@asimonov I looked into the core/include/ikos/core/domain/machine_int/operator.hpp and core/include/ikos/core/semantic/indexable.hpp files, it appears both of them already had cstdint included. However just to make sure the change takes place, I added a comment after cstdint to indicate this Issue.

I think once Dr. @ivanperez-keera merges this PR, it should allow ikos to build successfully in Ubuntu 24.04.

ivanperez-keera commented 2 months ago

@Mechazo11 . Thanks!

I don't think we need to add a comment pointing to the issue from any of the files. It's enough to just add the include where necessary (so, only analyzer/include/ikos/analyzer/analysis/liveness.hpp needs to change).

We can modify the .gitignore to include .vscode, but it's best to do that in a separate PR because that change is really not related to #273 (you just happened to run into that problem while trying to fix #273). We will not need an additional CLA for that separate PR if you send it, @Mechazo11 . Thanks!

Mechazo11 commented 2 months ago

Applied changes as suggested.

ivanperez-keera commented 2 months ago

Thanks! Will merge ASAP.

asimonov commented 2 months ago

@asimonov I looked into the core/include/ikos/core/domain/machine_int/operator.hpp and core/include/ikos/core/semantic/indexable.hpp files, it appears both of them already had cstdint included. However just to make sure the change takes place, I added a comment after cstdint to indicate this Issue.

I think once Dr. @ivanperez-keera merges this PR, it should allow ikos to build successfully in Ubuntu 24.04.

@Mechazo11 ok, may be it is already in latest version. in space-ros we use v3.2, I think, which probably dont have those changes.

thank you!

asimonov commented 2 months ago

Thanks! Will merge ASAP.

Ivan, after your merge I can try to build within space-ros on ubuntu24 and if it all works, then can you create a new tag for us to reference for jazzy upgrade of space-ros?

ivanperez-keera commented 2 months ago

The Mac build broke, so I'm checking what's going on there. It's an unrelated and documented error. I'll try to fix that one before this is merges so that all merges have completed a successful build run.

It seems to be working on Linux (based on the successful GA run).

asimonov commented 2 months ago

btw, this PR version works inside space-ros Earthfile build. so, would be good to have a version tag for it for Jazzy/Noble upgrade

ivanperez-keera commented 2 months ago

I'm waiting on the lawyer to give the ok (it's the last step before it can be merged; future contributions from the same author won't require this).

Wrt to the tag, I'm expecting to make a release of IKOS before the next release of Space ROS. That's the tag that should be used.

asimonov commented 2 months ago

I'm waiting on the lawyer to give the ok (it's the last step before it can be merged; future contributions from the same author won't require this).

Wrt to the tag, I'm expecting to make a release of IKOS before the next release of Space ROS. That's the tag that should be used.

@ivanperez-keera any news on this?

ivanperez-keera commented 2 months ago

Hi. I spoke with the lawyers several times this week (last time Wed) to process the CLA. I'm told it's in the last stage and should be approved soon. I'll ping again on Monday.

ivanperez-keera commented 2 months ago

I just received the ok. I'll make a few changes and merge. Thank you all for your patience and thanks @Mechazo11 for your contribution, and sorry that it took so long. It's just because it was the first time, but future contributions will be quicker to process.

I'll ping you when it's merged.

Mechazo11 commented 2 months ago

Dr. @ivanperez-keera no problem, I am very happy that I was able to help out.

ivanperez-keera commented 1 month ago

Your change has been merged! Thank you once again for your contribution to ikos! I hope you contribute more in the future. We could use your help :)

Mechazo11 commented 1 month ago

Dr. @ivanperez-keera it was my pleasure to help out. I may have found another fatal error in the base space-ros image. Will be opening an issue about it today.

With best regards, @Mechazo11