Avoid integer truncation

[reuk]

The build is broken on 32-bit platforms, which is important because we should support OS X universal binaries. This patch fixes the build errors by explicitly using 64-bit integers rather than depending on size_t to be 64 bits wide.

[reuk]

The return value here gets truncated to 0 when size_t is 32 bits wide.

[tautschnig]

Good catch; shouldn't we really be using mp_integer here?

[tautschnig]

There are actually more problems in this code:

• interpretert::unbounded_size does not consider unions or symbol types
• interpretert::get_size also uses unsigned internally.

I believe unbounded objects need to be handled properly at the call site of get_size.

[kroening]

Yes, mp_integer exists exactly to avoid such problems on all architectures.

[reuk]

I started looking at this, but it seems like changing that one function to return mp_integer will lead to a great number of further alterations. For example, it in turn requires that base_address_to_actual_size(std::size_t) be changed to take an mp_integer argument, which is then passed to memory.find(address), where memory is a sparse_vector. Either sparse_vector will need a find taking mp_integer, or we will need to report an error if the mp_integer is truncated when converting it to uint64_t.

Maybe this would be better addressed by someone who is already familiar with this code.

[tautschnig]

Maybe this would be better addressed by someone who is already familiar with this code.

Looking at the commit subjects, without actually having done a git blame: @romainbrenguier ?

[reuk]

Would it be worth merging this fix in now, and filing an issue to do a proper cleanup? At the moment, we don't have a working 32-bit build, and this patch at least resolves the immediate problem.

[tautschnig]

From a plain technical point of view, lacking all the management insight: do the proper fix, even if it means rewriting several functions. If questions about the code arise, git blame will tell whom to ask.

[tautschnig]

Thank you for the further work! I think all instances of uint64_t should go away, though: types are either compile-platform types (std::size_t) whenever the limitations of the runtime environment apply, or be unbounded (mp_integer).

[reuk]

I've tried to remove all instances of unsigned and uint64_t from the interpreter files. Use of auto is deliberate. It ensures that the correct numeric type is used, regardless of refactorings like these (this patch would have been much smaller if auto was used in the first place).

The guiding rationale has been to leave collection types unchanged. Conversions from mp_integer to size_t occur when the value is used as an index into an array. Otherwise, we try to prefer mp_integer.

[tautschnig]

I'll be taking a proper look in the morning - does this now require #1341 to build? (Which I'd be fine with, would just like to understand the CI failures.)

[reuk]

Yes, this depends on having properly-converting operators on big-int and therefore requires #1341

[reuk]

#1341 blocked indefinitely, closing this.

[tautschnig]

Joining my to-do list.

[romainbrenguier]

@tautschnig

Maybe this would be better addressed by someone who is already familiar with this code.

Looking at the commit subjects, without actually having done a git blame: @romainbrenguier ?

Actually @smowton knows that code better

[tautschnig]

@reuk: #1341 has been merged, would you be able to look into this one again? Please let me know how I can be of help, if at all.

[kroening]

I think that mp_integer is the right answer for the addresses; we do so everywhere else, and the cost isn't significantly higher than that of the 64-bit integers.

[tautschnig]

Closing in favour of #1484, which seems to be doing almost the same. (I don't know why this one hasn't been amended rather than doing duplicate work.)