AsmParser: Require null terminator in the created memory buffer.

[arphaman]

Summary

This patch modifies the memory buffer creation in the AsmParser library so that is requires a terminating null character.
This change is needed because LLLexer in AsmParser checks for EOF when it sees the null character, but the memory buffer
that's being created with the source assumes that the terminating null character isn't required.

I was motivated to submit this patch as my initial MIR commit had the problem where the input to the AsmParser wasn't null
terminated and it would have been better to catch it with an assertion early on.

[MatzeB]

That's unfortunate. It's probably better to change LLLexer to not require a 0 at the end. From what I can see the interface takes a StringRef which has a length and is not necessarily 0-terminated anyway.

[arphaman]

I updated the patch to use a range check in the LLLexer instead of relying on the null character for EOF.

[arphaman]

I think I'm wrong here actually, while this does work, it's not enough as a
lot of other code in LLLexer as it assumes no range checks.
It would probably be better to go with the original patch with a null
terminating character, as the LLLexer will require a lot of range
checks to be bug free when working with the non-null terminated input.

2015-05-20 12:36 GMT-07:00 Alex Lorenz <[e-mail]>:

I updated the patch to use a range check in the LLLexer instead of relying
on the null character for EOF.

REPOSITORY

rL LLVM

http://reviews.llvm.org/D9883

Files:

lib/AsmParser/LLLexer.cpp
unittests/AsmParser/AsmParserTest.cpp
unittests/AsmParser/CMakeLists.txt
unittests/AsmParser/Makefile
unittests/CMakeLists.txt
unittests/Makefile

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/

[arphaman]

I went back to the patch that requires a terminating null character again, as the range
checks in LLLexer would have to be introduced in a lot of places.

I updated the unittest to include a passing testcase as Duncan suggested.

[MatzeB]

Thanks for checking. This LGTM if Matthias agrees.

I still think LLLexer should be changed in the long term to use getNextChar() consistently and a range check included in it. While technically it can be indeed faster to check for a sentinel value like '\0' in this case the check was not included in the relevant switches but was a separate check in getNextChar() anyway, so I don't think it would have had any impact; Even if we would get rid of the check I'd be surprised if you could actually measure the difference.

Anyway I can understand that we may not want to change it right now, so this patch LGTM.

[arphaman]

2015-05-20 13:27 GMT-07:00 Matthias Braun <[e-mail]>:

Thanks for checking. This LGTM if Matthias agrees.

I still think LLLexer should be changed in the long term to use
getNextChar() consistently and a range check included in it. While
technically it can be indeed faster to check for a sentinel value like '\0'
in this case the check was not included in the relevant switches but was a
separate check in getNextChar() anyway, so I don't think it would have had
any impact; Even if we would get rid of the check I'd be surprised if you
could actually measure the difference.

Anyway I can understand that we may not want to change it right now, so
this patch LGTM.

Thanks, I committed the patch in r237833.

REPOSITORY

rL LLVM

http://reviews.llvm.org/D9883

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/

lib/AsmParser/LLLexer.cpp

 
 LLLexer::LLLexer(StringRef StartBuf, SourceMgr &sm, SMDiagnostic &Err,
                  LLVMContext &C)
   : CurBuf(StartBuf), ErrorInfo(Err), SM(sm), Context(C), APFloatVal(0.0) {
   CurPtr = CurBuf.begin();
 }
 
 int LLLexer::getNextChar() {
-  char CurChar = *CurPtr++;
-  switch (CurChar) {
-  default: return (unsigned char)CurChar;
-  case 0:
-    // A nul character in the stream is either the end of the current buffer or
-    // a random nul in the file.  Disambiguate that here.
-    if (CurPtr-1 != CurBuf.end())
-      return 0;  // Just whitespace.
-
-    // Otherwise, return end of file.
-    --CurPtr;  // Another call to lex will return EOF again.
+  if (CurPtr >= CurBuf.end())
     return EOF;
-  }
+  return (unsigned char)*CurPtr++;
 }
 
 
 lltok::Kind LLLexer::LexToken() {
   TokStart = CurPtr;
 
   int CurChar = getNextChar();
   switch (CurChar) {

unittests/AsmParser/AsmParserTest.cpp

+//===- llvm/unittest/AsmParser/AsmParserTest.cpp - asm parser unittests ---===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/AsmParser/Parser.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Support/SourceMgr.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+
+TEST(AsmParserTest, NonNullTerminatedInput) {
+  LLVMContext &Ctx = getGlobalContext();
+  StringRef Source = "; Empty module \n\1\2";
+  SMDiagnostic Error;
+  auto Mod =
+      parseAssemblyString(Source.substr(0, Source.size() - 2), Error, Ctx);
+
+  EXPECT_TRUE(Mod != nullptr);
+  EXPECT_TRUE(Error.getMessage().empty());
+}
+
+} // end anonymous namespace

unittests/AsmParser/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS
+  AsmParser
+  Core
+  Support
+  )
+
+add_llvm_unittest(AsmParserTests
+  AsmParserTest.cpp
+  )

unittests/AsmParser/Makefile

+##===- unittests/Assembler/Makefile ------------------------*- Makefile -*-===##
+#
+#                     The LLVM Compiler Infrastructure
+#
+# This file is distributed under the University of Illinois Open Source
+# License. See LICENSE.TXT for details.
+#
+##===----------------------------------------------------------------------===##
+
+LEVEL = ../..
+TESTNAME = AsmParser
+LINK_COMPONENTS := AsmParser Core Support
+
+include $(LEVEL)/Makefile.config
+include $(LLVM_SRC_ROOT)/unittests/Makefile.unittest

unittests/CMakeLists.txt

 add_custom_target(UnitTests)
 set_target_properties(UnitTests PROPERTIES FOLDER "Tests")
 
 if (APPLE)
   set(CMAKE_INSTALL_RPATH "@executable_path/../../lib")
 else(UNIX)
   set(CMAKE_INSTALL_RPATH "\$ORIGIN/../../lib${LLVM_LIBDIR_SUFFIX}")
 endif()
 
 function(add_llvm_unittest test_dirname)
   add_unittest(UnitTests ${test_dirname} ${ARGN})
 endfunction()
 
 add_subdirectory(ADT)
 add_subdirectory(Analysis)
+add_subdirectory(AsmParser)
 add_subdirectory(Bitcode)
 add_subdirectory(CodeGen)
 add_subdirectory(DebugInfo)
 add_subdirectory(ExecutionEngine)
 add_subdirectory(IR)
 add_subdirectory(LineEditor)
 add_subdirectory(Linker)
 add_subdirectory(MC)
 add_subdirectory(Option)
 add_subdirectory(ProfileData)
 add_subdirectory(Support)
 add_subdirectory(Transforms)

unittests/Makefile

 ##===- unittests/Makefile ----------------------------------*- Makefile -*-===##
 #
 #                     The LLVM Compiler Infrastructure
 #
 # This file is distributed under the University of Illinois Open Source
 # License. See LICENSE.TXT for details.
 #
 ##===----------------------------------------------------------------------===##
 
 LEVEL = ..
 
-PARALLEL_DIRS = ADT Analysis Bitcode CodeGen DebugInfo ExecutionEngine IR \
-    LineEditor Linker MC Option ProfileData Support Transforms
+PARALLEL_DIRS = ADT Analysis AsmParser Bitcode CodeGen DebugInfo \
+                ExecutionEngine IR LineEditor Linker MC Option ProfileData \
+                Support Transforms
 
 include $(LEVEL)/Makefile.config
 include $(LLVM_SRC_ROOT)/unittests/Makefile.unittest
 
 clean::
   $(Verb) $(RM) -f *Tests