fix maximum object size semantics #18728
Conversation
thestinger
changed the title
|
(It seems I can't do it due to github permissions...) |
|
max_obj_size is exclusive, so you can't create an object of size (1<<31) even without your patch. We may want to improve the name/comments through. |
|
@arielb1: I think |
|
@nikomatsakis, I believe bors will notice an r- comment after an r+, to stop the merge. |
|
Yeah I tried that later :) Niko -------- Original message -------- @nikomatsakis, I believe bors will notice an r- comment after an r+, to stop the merge. — |
| // Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or | ||
| // http://www.apache.org/licenses/LICENSE-2.0> or the MIT license | ||
| // <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
| // option. This file may not be copied, modified, or distributed |
| platform's pointer type. It can represent every memory address in the process. | ||
|
|
||
| The `int` type is a signed integer type with the same number of bits as the | ||
| platform's pointer type. The theoretical upper object on object size and array |
There was a problem hiding this comment.
"Upper object" is still here and still makes no sense.
|
(Cycling to stop bors.) |
|
(I think it may be a good idea to ask for a re-review next time. :) ) |
| // represent object size in bits. It is currently conservatively bounded to 1 << 47 as that is | ||
| // enough to cover the current usable address space on 64-bit ARMv8 and x86_64. | ||
| pub fn obj_size_bound(&self) -> u64 { | ||
| match self.sess().target.target.target_word_size[] { |
There was a problem hiding this comment.
Can you either add Fixes #18069 or keep the FIXME #18069 but change the text to "Investigate LLVM support for huge objects more deeply"? I doubt we've done the investigation, but it also seems like one of those open-ended issues that can never truly be closed (even if we DID exhaustively read each line of LLVM, it will keep being edited), so I'm not sure it's worth keeping around. Matching clang's limits might help give us confidence, of course we can't do that programmatically.
|
OK, modulo nits this seems good to me. @thestinger I think you mentioned that clang is much more conservative here? What limits do they use? Is there a good reason not to just match them? |
|
Ping @arielb1, I'd like your opinion on the revised commit as well. You mentioned in #18069 a certain amount of trepidation regarding LLVM internals. (See my comments here though; in the absence of documented LLVM limits, at some point we have to trust them.) |
|
On Fri, Nov 07, 2014 at 12:47:45PM -0800, Daniel Micay wrote:
I missed this before: can you elaborate on what you see as the |
|
LLVM has serious problems with objects of size The new test should use |
|
@arielb1 weren't there some compile-fail tests that would need to be updated to account for the fact that this patch permits larger objects than before? |
|
@arielb1 regarding exclusivity, that makes sense. I tend not to make assumptions about whether words like "max" or "bound" are inclusive or exclusive but I agree that bound is less ambiguous. |
|
@thestinger any plans to submit an updated version of this PR? seems like it's just some naming / english nits blocking it. |
internal: Split `serde` derive feature into `serde_derive` usage
This fixes the gap in the language definition causing #18726 by defining
a clear bound on the maximum size for libraries to enforce.
Closes #18069