Page MenuHomePhabricator

Set MemoryBuffer's RequiresNullTerminator to false by default.
AcceptedPublic

Authored by ruiu on Apr 27 2018, 12:27 PM.

Details

Summary

MemoryBuffer::getFile()-family functions take RequiresNullTerminator
boolean argument. If it is true, MemoryBuffer ensures that the buffer
ends with '\0'. By default, it is true.

But that default was not reasonable. If you are reading binary files,
or even if you are reading text files, you usually don't need a nul
terminator character. However, because it's default, we do that in many
places.

This patch flips the default so that we don't append '\0' by default.

I tested this patch by adding code to add a dummy non-'\0' byte at end
of a buffer if RequiresNullTerminator is false and run all tests.

Event Timeline

ruiu created this revision.Apr 27 2018, 12:27 PM

I really like the idea of changing the default, but maybe instead of an argument we should just have two functions:

getFile(...)
getNullTerminatedFile(...)

clang/tools/clang-refactor/TestSupport.cpp
297

Maybe we should have a getNullTerminatedFile?

llvm/unittests/AsmParser/AsmParserTest.cpp
36

Why delete this test?

ruiu added inline comments.Apr 30 2018, 9:38 AM
clang/tools/clang-refactor/TestSupport.cpp
297

I tried that but it seems it is better to do it incrementally. MemoryBuffer functions are used at a lot of places in LLVM and doing multiple changes in the same patch isn't easy. So let me submit this patch first.

llvm/unittests/AsmParser/AsmParserTest.cpp
36

Because this test doesn't pass with this patch and what this test is testing doesn't make sense. It verifies that the asm parser dies with an assertion failure if an input is not \0-terminated, but the asm parser doesn't actually requires the terminator. It works fine without it.

Thanks for doing this! I did some of the ground work for this in the COFF linker, but never got around to flipping the default. I too like the idea of having a separate function for the NUL-terminated case. It seems we all want that, so how do we get there? Are you planning to follow this up with a patch that removes the RequiresNullTerminator flag and rewrites calls that use it to use the new function instead?

ruiu added a comment.Jun 8 2018, 3:16 PM

I believe this patch is ready for submit, though I didn't get any LGTM for this. Could you review it for me?

I haven't accepted the patch yet because I want to make sure we do end up removing the RequiresNullTerminator flag. You indicated that you don't want to do that in this patch. That's ok with me as long as you do plan to do it in a soon-to-come follow-up patch. If you don't plan to write that follow-up patch, I would like you to include the new function and rewrite the calls in this patch.

ruiu added a comment.Jun 8 2018, 4:33 PM

Yes, I'll create a follow-up patch to remove RequiresNullTerminator from this function and create a new function for those that needs that functionality.

inglorion accepted this revision.Jun 8 2018, 5:04 PM

Alright then, LGTM.

This revision is now accepted and ready to land.Jun 8 2018, 5:04 PM