This is an archive of the discontinued LLVM Phabricator instance.

[support] remove tautological comparison in Support/Windows/Path.inc
ClosedPublic

Authored by inglorion on Oct 24 2017, 4:51 PM.

Details

Summary

The removed code checks that we are able to handle a 64-bit number, but
the code we're calling takes two dwords (for a total of 64 bits), so this
is always true.

Event Timeline

inglorion created this revision.Oct 24 2017, 4:51 PM
zturner edited edge metadata.

Adding more Windows people, because as I look over this code, it actually looks wrong to me. The value we pass for MaximumFileSize seems unnecessarily large. I'm going through the docs at the moment to figure out the correct thing to do here.

Let me know if this is correct. I think we can actually get (potentially very large) savings from this.

llvm/lib/Support/Windows/Path.inc
741–742

I don't think we need Offset as part of this calculation. If we want to map 32 bytes starting at offset 32, the current code will create a file mapping object with a maximum size of 64. This doesn't seem necessary. It should be creating a file mapping object of exactly 32 bytes, just like we requested. So, I propose changing this line to: CreateFileMappingW(FileHandle, 0, flprotect, Size >> 32, Size & 0xFFFFFFFF);

755–759

This part does not need to be changed. because the "View" is a disconnected entity from the "Mapping", This will map Size bytes from file offset Offset into offset 0 of the mapping object.

This is no different from what happened before, except that before our mapping object would just get a ton of unnecessary bytes allocated at the end of it.

Zach is correct about the overly large reservation in the file mapping and we should fix that too.

llvm/lib/Support/Windows/Path.inc
726

I assume someone was originally worried that SIZE_T would be 32 bits wide in a 32-bit build but somehow believed std::size_t (which is the type of Size) could still somehow be larger than that. Ain't gonna happen.

rnk added inline comments.Oct 25 2017, 8:54 AM
llvm/lib/Support/Windows/Path.inc
726

I wonder if we should turn these examples of dynamic range checks into static_asserts. We saw a lot of these tautological comparison warnings in Chrome, where it was much less obvious that the types are the same size. Would this be an appropriate replacement for the check?

static_assert(std::numeric_limits<decltype(Size)>::max() <=
              std::numeric_limits<SIZE_T>::max());

@zturner do you want me to make the semantic changes in the same commit or can we ship the size check removal first and then change the functionality? The latter would be my preference.

llvm/lib/Support/Windows/Path.inc
726

My original plan was to come up with a way to elide the check if we can statically verify that it will fit, but then zturner pointed out that this is always true and we can just delete the check altogether.

Note that the code checks if Size fits in a SIZE_T. Size was a uint64_t until recently and is now a size_t, which is at most a 64-bit unsigned. Offset is already an uint64_t. So we need a 64-bit value even if Size were 32-bit on some system. And we do have 64 bits, because we're passing two 32-bit values to CreateFileMappingW and MapViewOfFile. So a 64-bit value will always fit.

Is this good to go or does it need changes?

zturner accepted this revision.Oct 27 2017, 2:08 PM
This revision is now accepted and ready to land.Oct 27 2017, 2:08 PM
This revision was automatically updated to reflect the committed changes.

@zturner do you want me to make the semantic changes in the same commit or can we ship the size check removal first and then change the functionality? The latter would be my preference.

Just want to confirm that this follow up change is still going to happen? This is what I was worried about with letting it go in separately. I really don't want to see this change forgotten about, as it seems like a very useful improvement.

I would never forget about...err, what was it again I needed to remember? Thanks for the reminder. Follow-up change is in D39876.