[Preprocessor]Correct Macro-Arg allocation of StringifiedArguments, correct getNumArguments

[erichkeane]

Summary

StringifiedArguments is allocated (resized) based on the size the getNumArguments function. However, this function ACTUALLY currently returns the amount of total UnexpArgTokens which is minimum the same as the new implementation of getNumMacroArguments, since empty/omitted arguments result in 1 UnexpArgToken, and included ones at minimum include 2 (1 for the arg itself, 1 for eof).

This patch renames the otherwise unused getNumArguments to be more clear that it is the number of arguments that the Macro expects, and thus the maximum number that can be stringified. This patch also replaces the explicit memset (which results in value instantiation of the new tokens, PLUS clearing the memory) with brace initialization.

[erichkeane]

Bump! I'll note that this patch has nothing to do with MSVC and should simply be a memory-usage savings.

[erichkeane]

Bump :)

[erichkeane]

Anyone have a chance to look at this? I'll note that this is in no way MSVC specific, this is a code-health issue.

[arphaman]

Can you somehow test this change? Maybe a unittest for getStringifiedArgumentthat ensures that the assertion is triggered after the number of macro arguments is reached?

[erichkeane]

Sure, I'll work on that, thanks!

[erichkeane]

Added unit test to validate that this still works throughout the range of arguments, and asserts as soon as you get to the end as requested.

[arphaman]

LGTM! Please fix the code style issues that I pointed out before committing.

unittests/Lex/LexerTest.cpp

24

Please put these two new includes before #include "clang/Lex/ModuleLoader.h"

46

I think this violates the 80 columns, please reformat this declaration.

62

NIT: Please add a new line after '}'.

[kcc]

the bots complain about a leak in the new test code.
Please fix/revert ASAP.
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/5691/steps/check-clang%20asan/logs/stdio

28905==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 216 byte(s) in 1 object(s) allocated from:

#0 0x4eca08 in __interceptor_malloc /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:66
#1 0xefcb8f in clang::MacroArgs::create(clang::MacroInfo const*, llvm::ArrayRef<clang::Token>, bool, clang::Preprocessor&) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/Lex/MacroArgs.cpp:51:27
#2 0x54dc56 in (anonymous namespace)::LexerTest_DontOverallocateStringifyArgs_Test::TestBody() /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/unittests/Lex/LexerTest.cpp:405:19
#3 0x65154e in HandleExceptionsInMethodIfSupported<testing::Test, void> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/googletest/src/gtest.cc:2458:12
#4 0x65154e in testing::Test::Run() /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/googletest/src/gtest.cc:2474
#5 0x653848 in testing::TestInfo::Run() /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/googletest/src/gtest.cc:2656:11
#6 0x654b86 in testing::TestCase::Run() /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/googletest/src/gtest.cc:2774:28
#7 0x675586 in testing::internal::UnitTestImpl::RunAllTests() /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/googletest/src/gtest.cc:4649:43
#8 0x67487e in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/googletest/src/gtest.cc:2458:12
#9 0x67487e in testing::UnitTest::Run() /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/googletest/src/gtest.cc:4257
#10 0x634bfe in RUN_ALL_TESTS /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/googletest/include/gtest/gtest.h:2233:46
#11 0x634bfe in main /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/UnitTestMain/TestMain.cpp:51
#12 0x7f016e9cb82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

include/clang/Lex/MacroArgs.h

class MacroArgs {
   /// StringifiedArgs - This contains arguments in 'stringified' form.  If the
   /// stringified form of an argument has not yet been computed, this is empty.
   std::vector<Token> StringifiedArgs;
 
   /// ArgCache - This is a linked list of MacroArgs objects that the
   /// Preprocessor owns which we use to avoid thrashing malloc/free.
   MacroArgs *ArgCache;
 
-  MacroArgs(unsigned NumToks, bool varargsElided)
+  /// MacroArgs - The number of arguments the invoked macro expects.
+  unsigned NumMacroArgs;
+
+  MacroArgs(unsigned NumToks, bool varargsElided, unsigned MacroArgs)
       : NumUnexpArgTokens(NumToks), VarargsElided(varargsElided),
-      ArgCache(nullptr) {}
+        ArgCache(nullptr), NumMacroArgs(MacroArgs) {}
   ~MacroArgs() = default;
 
 public:
   /// MacroArgs ctor function - Create a new MacroArgs object with the specified
   /// macro and argument info.
   static MacroArgs *create(const MacroInfo *MI,
                            ArrayRef<Token> UnexpArgTokens,
                            bool VarargsElided, Preprocessor &PP);
const std::vector<Token> &
     getPreExpArgument(unsigned Arg, const MacroInfo *MI, Preprocessor &PP);
 
   /// getStringifiedArgument - Compute, cache, and return the specified argument
   /// that has been 'stringified' as required by the # operator.
   const Token &getStringifiedArgument(unsigned ArgNo, Preprocessor &PP,
                                       SourceLocation ExpansionLocStart,
                                       SourceLocation ExpansionLocEnd);
 
-  /// getNumArguments - Return the number of arguments passed into this macro
-  /// invocation.
-  unsigned getNumArguments() const { return NumUnexpArgTokens; }
-
+  /// getNumMacroArguments - Return the number of arguments the invoked macro
+  /// expects.
+  unsigned getNumMacroArguments() const { return NumMacroArgs; }
 
   /// isVarargsElidedUse - Return true if this is a C99 style varargs macro
   /// invocation and there was no argument specified for the "..." argument.  If
   /// the argument was specified (even empty) or this isn't a C99 style varargs
   /// function, or if in strict mode and the C99 varargs macro had only a ...
   /// argument, this returns false.
   bool isVarargsElidedUse() const { return VarargsElided; }
 

lib/Lex/MacroArgs.cpp

if ((*Entry)->NumUnexpArgTokens >= UnexpArgTokens.size() &&
       ResultEnt = Entry;
       
       // If we have an exact match, use it.
       if ((*Entry)->NumUnexpArgTokens == UnexpArgTokens.size())
         break;
       // Otherwise, use the best fit.
       ClosestMatch = (*Entry)->NumUnexpArgTokens;
     }
-  
   MacroArgs *Result;
   if (!ResultEnt) {
     // Allocate memory for a MacroArgs object with the lexer tokens at the end.
-    Result = (MacroArgs*)malloc(sizeof(MacroArgs) + 
+    Result = (MacroArgs *)malloc(sizeof(MacroArgs) +
                                  UnexpArgTokens.size() * sizeof(Token));
     // Construct the MacroArgs object.
-    new (Result) MacroArgs(UnexpArgTokens.size(), VarargsElided);
+    new (Result)
+        MacroArgs(UnexpArgTokens.size(), VarargsElided, MI->getNumArgs());
   } else {
     Result = *ResultEnt;
     // Unlink this node from the preprocessors singly linked list.
     *ResultEnt = Result->ArgCache;
     Result->NumUnexpArgTokens = UnexpArgTokens.size();
     Result->VarargsElided = VarargsElided;
+    Result->NumMacroArgs = MI->getNumArgs();
   }
 
   // Copy the actual unexpanded tokens to immediately after the result ptr.
   if (!UnexpArgTokens.empty())
     std::copy(UnexpArgTokens.begin(), UnexpArgTokens.end(), 
               const_cast<Token*>(Result->getUnexpArgument(0)));
 
   return Result;

 }
 
 /// getStringifiedArgument - Compute, cache, and return the specified argument
 /// that has been 'stringified' as required by the # operator.
 const Token &MacroArgs::getStringifiedArgument(unsigned ArgNo,
                                                Preprocessor &PP,
                                                SourceLocation ExpansionLocStart,
                                                SourceLocation ExpansionLocEnd) {
-  assert(ArgNo < NumUnexpArgTokens && "Invalid argument number!");
-  if (StringifiedArgs.empty()) {
-    StringifiedArgs.resize(getNumArguments());
-    memset((void*)&StringifiedArgs[0], 0,
-           sizeof(StringifiedArgs[0])*getNumArguments());
-  }
+  assert(ArgNo < getNumMacroArguments() && "Invalid argument number!");
+  if (StringifiedArgs.empty())
+    StringifiedArgs.resize(getNumMacroArguments(), {});
+
   if (StringifiedArgs[ArgNo].isNot(tok::string_literal))
     StringifiedArgs[ArgNo] = StringifyArgument(getUnexpArgument(ArgNo), PP,
                                                /*Charify=*/false,
                                                ExpansionLocStart,
                                                ExpansionLocEnd);
   return StringifiedArgs[ArgNo];
 }

unittests/Lex/LexerTest.cpp

 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/HeaderSearchOptions.h"
 #include "clang/Lex/ModuleLoader.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
+#include "clang/Lex/MacroArgs.h"
+#include "clang/Lex/MacroInfo.h"
 #include "gtest/gtest.h"
 
 using namespace clang;
 
 namespace {
 
 // The test fixture.
 class LexerTest : public ::testing::Test {
 protected:
   LexerTest()
     : FileMgr(FileMgrOpts),
       DiagID(new DiagnosticIDs()),
       Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
       SourceMgr(Diags, FileMgr),
       TargetOpts(new TargetOptions) 
   {
     TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
     Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
   }
 
-  std::vector<Token> Lex(StringRef Source) {
+  std::unique_ptr<Preprocessor> CreatePP(StringRef Source, TrivialModuleLoader& ModLoader) {
     std::unique_ptr<llvm::MemoryBuffer> Buf =
         llvm::MemoryBuffer::getMemBuffer(Source);
     SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(Buf)));
 
-    TrivialModuleLoader ModLoader;
     MemoryBufferCache PCMCache;
     HeaderSearch HeaderInfo(std::make_shared<HeaderSearchOptions>(), SourceMgr,
                             Diags, LangOpts, Target.get());
-    Preprocessor PP(std::make_shared<PreprocessorOptions>(), Diags, LangOpts,
-                    SourceMgr, PCMCache, HeaderInfo, ModLoader,
-                    /*IILookup =*/nullptr,
-                    /*OwnsHeaderSearch =*/false);
-    PP.Initialize(*Target);
-    PP.EnterMainSourceFile();
+    std::unique_ptr<Preprocessor> PP = llvm::make_unique<Preprocessor>(
+        std::make_shared<PreprocessorOptions>(), Diags, LangOpts, SourceMgr,
+        PCMCache, HeaderInfo, ModLoader,
+    PP->Initialize(*Target);
+    PP->EnterMainSourceFile();
+    return PP;
+  }
+  std::vector<Token> Lex(StringRef Source) {
+    TrivialModuleLoader ModLoader;
+    auto PP = CreatePP(Source, ModLoader);
 
     std::vector<Token> toks;
     while (1) {
       Token tok;
-      PP.Lex(tok);
+      PP->Lex(tok);
       if (tok.is(tok::eof))
         break;
       toks.push_back(tok);
     }
 
     return toks;
   }
 
TEST_F(LexerTest, DontMergeMacroArgsFromDifferentMacroFiles) {
   //
   // The lexer used to report its size as 31, meaning that the end of the
   // expansion would be on the *next line* (just past `M2("a", "b")`). Make
   // sure that we get the correct end location (the comma after "helper1").
   SourceLocation helper1ArgLoc = toks[20].getLocation();
   EXPECT_EQ(SourceMgr.getFileIDSize(SourceMgr.getFileID(helper1ArgLoc)), 8U);
 }
 
+TEST_F(LexerTest, DontOverallocateStringifyArgs) {
+  TrivialModuleLoader ModLoader;
+  auto PP = CreatePP("\"StrArg\", 5, 'C'", ModLoader);
+
+  llvm::BumpPtrAllocator Allocator;
+  std::array<IdentifierInfo *, 3> ArgList;
+  MacroInfo *MI = PP->AllocateMacroInfo({});
+  MI->setIsFunctionLike();
+  MI->setArgumentList(ArgList, Allocator);
+  EXPECT_EQ(3, MI->getNumArgs());
+  EXPECT_TRUE(MI->isFunctionLike());
+
+  Token Eof;
+  Eof.setKind(tok::eof);
+  std::vector<Token> ArgTokens;
+  while (1) {
+    Token tok;
+    PP->Lex(tok);
+    if (tok.is(tok::eof)) {
+      ArgTokens.push_back(Eof);
+      break;
+    }
+    if (tok.is(tok::comma))
+      ArgTokens.push_back(Eof);
+    else
+      ArgTokens.push_back(tok);
+  }
+
+  MacroArgs *MA = MacroArgs::create(MI, ArgTokens, false, *PP);
+  Token Result = MA->getStringifiedArgument(0, *PP, {}, {});
+  EXPECT_EQ(tok::string_literal, Result.getKind());
+  EXPECT_STREQ("\"\\\"StrArg\\\"\"", Result.getLiteralData());
+  Result = MA->getStringifiedArgument(1, *PP, {}, {});
+  EXPECT_EQ(tok::string_literal, Result.getKind());
+  EXPECT_STREQ("\"5\"", Result.getLiteralData());
+  Result = MA->getStringifiedArgument(2, *PP, {}, {});
+  EXPECT_EQ(tok::string_literal, Result.getKind());
+  EXPECT_STREQ("\"'C'\"", Result.getLiteralData());
+#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
+  EXPECT_DEATH(MA->getStringifiedArgument(3, *PP, {}, {}),
+               "Invalid argument number!");
+#endif
+}
+
 } // anonymous namespace