[llvm] r208021 - [Support/MemoryBuffer] Move the IsVolatile check inside shouldUseMmap() and make sure to zero-initialize the rest

[Argyrios Kyrtzidis]

On May 6, 2014, at 8:51 AM, Rafael Espíndola <rafael.espindola at gmail.com> wrote:

>>> This can be reached because of uses error, right? Editing a file at
>>> the right moment while clang is running for example. If that is the
>>> case, this should probably be a report_fatal_error instead of an
>>> assert.
>> 
>> Are you saying if IsVolatile is false it should crash the compiler ?
>> I'm not fond of crashing the compiler because of user actions that we know how to recover from.
> 
> The issue right now is that we behave differently with and without asserts:
> 
> * With asserts we crash.
> * Without asserts we zero part of the buffer.
> 
> Asserts should be reserved for things that only fire because we (llvm
> and clang developers) made a mistake. That is, in a bug. Maybe the
> correct solution is to just delete the assert?

I'm fine with removing the assert.

> I think the net result
> would be:
> 
> * IsVolatileSize forces us to use read instead of mmap.
> * If for any reason we use read, we can cope with the file size
> changing. The buffer itself will contain some mix of old and new data,
> but the null terminator will be there.
> * If using mmap we will have to report fatal errors since we cannot
> guarantee that the null terminator will be there if the file size
> changes.

How can we report fatal errors when using mmap ? mmap will map the page when the caller tries to read that memory part, at which point I don't see what we can do about it.

> 
> Cheers,
> Rafael