Skip to content
This repository was archived by the owner on Oct 3, 2021. It is now read-only.

Fixes for overflow issues #837

Merged
merged 10 commits into from
Oct 27, 2019
Merged

Fixes for overflow issues #837

merged 10 commits into from
Oct 27, 2019

Conversation

nguyenthanhvuh
Copy link
Contributor

  • programs added to new and appropriately named directory

  • license present and acceptable (either in separate file or as comment at beginning of program)

  • contributed-by present (either in README file or as comment at beginning of program)

  • programs added to a .set file of an existing category, or new sub-category established (if justified)

  • intended property matches the corresponding .prp file

  • programs and expected answer added to a .yml file according to task definitions

  • architecture (32 bit vs. 64 bit) matches the corresponding .cfg file
  • original sources present
  • preprocessed files present
  • preprocessed files generated with correct architecture
  • Makefile added with correct content and without overly broad suppression of warnings

@nguyenthanhvuh
Copy link
Contributor Author

nguyenthanhvuh commented Oct 19, 2019

Fixes for integer overflow issues mentioned in #813

@timosantonopoulos

@dbeyer
Copy link
Member

dbeyer commented Oct 22, 2019

@nguyenthanhvuh Did you see the CI results? Please ask if you need help / have questions.

@nguyenthanhvuh
Copy link
Contributor Author

@dbeyer yep, working on it

@nguyenthanhvuh
Copy link
Contributor Author

@dbeyer I've made some changes to fix the errors given by the previous CI build. But now the CI build has a timeout problem that I am not sure how to proceed. Could you help?

@dbeyer dbeyer requested a review from kfriedberger October 27, 2019 06:48
Copy link
Member

@kfriedberger kfriedberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including limits.h requires to add the preprocessed .i-files.
Please execute some comand like cpp -m32 -P FILE.c > FILE.i and add the new .i-files.
Additionally change the yml-files to reference the .i-files.
This should only be done for files that use macros, e.g. include statements.

UPDATE: directly done on my own.

Related topic: Why did the CI-check not detect this?

This avoids the need for a preprocessed version of the file.
and change corresponding benchmark yml files.
@kfriedberger
Copy link
Member

kfriedberger commented Oct 27, 2019

The changes replace several signed ints with overflow-safe unsinged ints and add limitation on the rnage of possible values. The changes seem to be valid.
I did a small change on top of the existing changes to avoid unneeded header-includes and I added the preprocessed files where required.

@dbeyer you can merge this PR, as soon as the CI passes.

@dbeyer
Copy link
Member

dbeyer commented Oct 27, 2019

Thanks a lot!

@dbeyer dbeyer merged commit 28a1f6d into sosy-lab:master Oct 27, 2019
@nguyenthanhvuh
Copy link
Contributor Author

@kfriedberger @dbeyer thank you.

@MartinSpiessl
Copy link
Contributor

Sorry, I must have missed this PR. One could have added the overflow verdict true to the fixed yml files, and kept the faulty versions as overflow tasks with a false verdict. Also, this does not fix the overflows completely (not even all the files with overflows have been changed), cf. #813.
If this was a work in progress I would suggest to add WIP: to the PR title, this way it does not get merged that fast by @dbeyer 😉.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants