Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct size of allocated space for unbounded objects #987

Conversation

mgudemann
Copy link
Contributor

On 32 bit data models, there was an overflow due to an unsigned long long
result cast into size_t, effectively preventing compilation on 32 bit
systems.

On 32 bit data models, there was an overflow due to an `unsigned long long`
result cast into `size_t`, effectively preventing compilation on 32 bit
systems.
Copy link
Contributor

@romainbrenguier romainbrenguier left a comment

Choose a reason for hiding this comment

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

This looks good to me

@mgudemann mgudemann self-assigned this Jun 3, 2017
@smowton
Copy link
Contributor

smowton commented Jun 5, 2017

Oops. Although, if size_t is being used to bound the total address space, does that mean such an unbounded object will more or less fill it on a 32-bit system? If so we should use uint64_t instead of size_t.

@mgudemann
Copy link
Contributor Author

@smowton I am not sure about this, before we had 2<<32 which amounts to 8G. Linux over-commits memory, but this may lead to problems on systems that actually only promise the memory they have. As we have to specify a bound for unbounded it could be anything that is somehow reasonable.

@smowton
Copy link
Contributor

smowton commented Jun 6, 2017

It doesn't actually allocate any memory, just interpreter address space. My concern is running out of address space, not memory.

@mgudemann
Copy link
Contributor Author

@smowton ok, then your proposal would be to use 64 bit interpreter address space on 32 bit systems, too?
wouldn't it make sense to see what size bound can be done reasonably and limit to this?

@smowton
Copy link
Contributor

smowton commented Jun 6, 2017

Yes re: using 64-bit interpreter addresses everywhere. Don't know about limits-- I picked 2^32 out of the air, but 2^16 seems likely to be routinely overflowed and 2^24 leaves rather few object addresses available on a 32-bit arch.

@mgudemann
Copy link
Contributor Author

@smowton @romainbrenguier do we agree that this should be done, but is not urgent to to 32 bit not being the main target architecture?

@smowton
Copy link
Contributor

smowton commented Jun 6, 2017

Yes

@romainbrenguier
Copy link
Contributor

yes

@peterschrammel
Copy link
Member

@mgudemann, rebase please

@peterschrammel peterschrammel requested a review from smowton July 13, 2017 19:25
@mgudemann
Copy link
Contributor Author

@peterschrammel please note that we weill currently not fix this, as only 32 bit systems are affected

@romainbrenguier
Copy link
Contributor

@mgudemann @smowton actually I think travis runs tests on some 32 bits systems, so this problem could lead to failures on CI that are difficult to debug. Could we get this merged? If the size is still a problem, can we pick something 2^16 and 2^24 and add a note saying this is an arbitrary limit?

@smowton
Copy link
Contributor

smowton commented Aug 7, 2017

Are you sure? I don't see any evidence of that in .travis.yml

@romainbrenguier
Copy link
Contributor

@smowton yes maybe you are right

@tautschnig tautschnig changed the base branch from test-gen-support to develop August 22, 2017 12:23
@smowton smowton removed their request for review September 1, 2017 09:18
@mgudemann
Copy link
Contributor Author

obsolete with #1484

@mgudemann mgudemann closed this Oct 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants