This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by erichkeane on Apr 13 2017, 1:57 PM.

Details

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.

Diff Detail

Event Timeline

erichkeane created this revision.Apr 13 2017, 1:57 PM
erichkeane retitled this revision from Correct Macro-Arg allocation of StringifiedArguments, correct getNumArguments to [Preprocessor]Correct Macro-Arg allocation of StringifiedArguments, correct getNumArguments.Apr 13 2017, 2:14 PM
erichkeane added reviewers: rnk, rsmith.
erichkeane added a subscriber: cfe-commits.

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

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.

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?

Sure, I'll work on that, thanks!

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 accepted this revision.Jun 14 2017, 2:14 PM

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 '}'.

This revision is now accepted and ready to land.Jun 14 2017, 2:14 PM
This revision was automatically updated to reflect the committed changes.
erichkeane marked 3 inline comments as done.
kcc added a subscriber: kcc.Jun 15 2017, 10:44 AM

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)