New issue
Advanced search Search tips

Issue 598141 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 578344



Sign in to add a comment

rewrite_to_chrome_style: unnamed args in function template declarations are getting rewritten

Project Member Reported by dcheng@chromium.org, Mar 26 2016

Issue description

Example fixup patch needed to make things compile:
     void Clear() { ShrinkCapacity(0); }
 
-    template <typename U> void Append(const U*, size_tdata_size;
+    template <typename U> void Append(const U*, size_t);
     template <typename U> void Append(U&&);
 

Comment 1 by dcheng@chromium.org, Mar 26 2016

Nothing particularly exciting:
| | | |-FunctionTemplateDecl 0x73e0988 <line:763:5, col:55> col:32 append
| | | | |-TemplateTypeParmDecl 0x73e06a0 <col:15, col:24> col:24 referenced typename U
| | | | `-CXXMethodDecl 0x73e08e0 <col:27, col:55> col:32 append 'void (const U *, size_t)'
| | | |   |-ParmVarDecl 0x73e0758 <col:39, col:46> col:47 'const U *'
| | | |   `-ParmVarDecl 0x73e07c8 <col:49> col:55 'size_t':'unsigned long'

However, the template declaration is defined later in the same file:
template <typename T, size_t inlineCapacity, typename Allocator>
template <typename U>
void Vector<T, inlineCapacity, Allocator>::append(const U* data, size_t dataSize)
{
    ASSERT(Allocator::isAllocationAllowed());
    size_t newSize = m_size + dataSize;
    if (newSize > capacity()) {
        data = expandCapacity(newSize, data);
        ASSERT(begin());
    }
    RELEASE_ASSERT(newSize >= m_size);
    T* dest = end();
    ANNOTATE_CHANGE_SIZE(begin(), capacity(), m_size, newSize);
    VectorCopier<VectorTraits<T>::canCopyWithMemcpy, T>::uninitializedCopy(data, &data[dataSize], dest);
    m_size = newSize;
}

So I guess that renaming that parameter is clobbering the one in the forward declaration? Here's the initial bit of the definition's AST:
| |-FunctionTemplateDecl 0x7410d00 parent 0x73d4e40 prev 0x73e0988 <line:1157:1, line:1171:1> line:1
158:44 append
| | |-TemplateTypeParmDecl 0x74106b0 <line:1157:11, col:20> col:20 referenced typename U
| | `-CXXMethodDecl 0x7410c20 parent 0x73d4e40 prev 0x73e08e0 <line:1156:1, line:1171:1> line:1158:44 append 'void (const U *, size_t)'
| |   |-ParmVarDecl 0x7410ad8 <col:51, col:60> col:60 referenced data 'const U *'
| |   |-ParmVarDecl 0x7410b48 <col:66, col:73> col:73 referenced dataSize 'size_t':'unsigned long'

No obvious conflicts that I can see here.

Comment 2 by dcheng@chromium.org, Mar 27 2016

Figured out how to repro it:
diff --git a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
index f07e500a..92e7cba 100644
--- a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
+++ b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
@@ -514,6 +514,20 @@ class RewriterBase : public MatchFinder::MatchCallback {
     clang::SourceLocation loc = TargetNodeTraits<TargetNode>::GetLoc(
         *result.Nodes.getNodeAs<TargetNode>(
             TargetNodeTraits<TargetNode>::GetName()));
+    // Probably shouldn't be replacing parens ever.
+    {
+      clang::Token tok;
+      if (!clang::Lexer::getRawToken(loc, tok, *result.SourceManager,
+                                     result.Context->getLangOpts(), false)) {
+        if (tok.getKind() == clang::tok::r_paren) {
+          fprintf(stderr, "Badness!\n");
+          decl->dump();
+          fprintf(stderr, "Target loc:\n");
+          loc.dump(*result.SourceManager);
+          fprintf(stderr, "\n");
+          assert(false);
+        }
+      }
+    }
     clang::CharSourceRange range = clang::CharSourceRange::getTokenRange(loc);
     replacements_->emplace(*result.SourceManager, range, new_name);
     replacement_names_.emplace(old_name.str(), std::move(new_name));

Rebuild and then run:
$ tools/clang/scripts/run_tool.py rewrite_to_chrome_style out/Debug third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp

Failed to process /usr/local/google/home/dcheng/src/chrome/src/third_party/WebKit/Source/core/testing/v8/WebCoreTestSupport.cpp
warning: ../../third_party/llvm-build/Release+Asserts/bin/clang++: 'linker' input unused
Badness!
ParmVarDecl 0xaa69f48 <../../third_party/WebKit/Source/wtf/Vector.h:763:49, col:55> col:55 used dataSize 'size_t':'unsigned long'
Target loc:
../../third_party/WebKit/Source/wtf/Vector.h:763:55
rewrite_to_chrome_style: /usr/local/google/home/dcheng/src/chrome/src/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:527: void {anonymous}::RewriterBase<DeclNode, TargetNode>::run(const clang::ast_matchers::MatchFinder::MatchResult&) [with DeclNode = clang::VarDecl; TargetNode = clang::NamedDecl]: Assertion `false' failed.

Succeeded: 0, Failed: 1, Edits: 0 [100.00%]
Applied 0 edits to 0 files

Going to try dumping the AST to see if there's anything interesting in there.

Comment 3 by dcheng@chromium.org, Mar 28 2016

OK, I guess this is coming from an instantiation of the template:

| | | |-FunctionTemplateDecl 0x870c328 <line:763:5, col:55> col:32 append
| | | | |-TemplateTypeParmDecl 0x870c088 <col:15, col:24> col:24 typename U
| | | | |-CXXMethodDecl 0x870c280 <col:27, col:55> col:32 append 'void (const U *, size_t)'
| | | | | |-ParmVarDecl 0x870c148 <col:39, col:46> col:47 'const U *'
| | | | | `-ParmVarDecl 0x870c1a8 <col:49> col:55 'size_t':'unsigned long'
| | | | `-CXXMethodDecl 0xba06590 <line:1158:1, line:1171:1> line:763:32 used append 'void (const char *, size_t)'
| | | |   |-TemplateArgument type 'char'
| | | |   |-ParmVarDecl 0xba06458 <col:39, col:47> col:47 used data 'const char *'
| | | |   |-ParmVarDecl 0xba064b8 <col:49, col:55> col:55 used dataSize 'size_t':'unsigned long'
| | | |   `-CompoundStmt 0xd9cb1e0 <line:1159:1, line:1171:1>

Not sure what to do about this. I guess we could add some special code to measure the length of the token, and bail out if it doesn't look right, but it's kind of icky.

Comment 4 by dcheng@chromium.org, Mar 28 2016

I still haven't been able to repro in a test, but the problematic instantiation seems to be coming from setMultipartBoundary() in https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/network/ResourceResponse.h&rcl=1459107294&l=218

Comment 5 by danakj@chromium.org, Mar 28 2016

Maybe we have to use getLocStart and End or whatever and look if there's a space or something?

Comment 6 by dcheng@chromium.org, Mar 28 2016

My initial attempts at fixing this were based on trying to determine if the end of the replacement source range (in character positions) was > the end of the original source range (in character positions) of the ParmVarDecl. It's a bit tricky, but basically:
- Build a character range from decl->getSourceRange() using clang::Lexer::getAsCharRange().
- Convert the replacement range to a character range as well.
- Compare the ending SourceLocations using isBeforeInTranslationUnit.
I don't know if I got my math wrong (quite possible), but I couldn't get it to work. I'll try another angle on this today.

Alternate approaches:
- use clang::Lexer::getRawToken() and make sure the token we're trying to replace is a clang::tok::identifier or clang::tok::raw_identifier (I think the latter is what we see with getRawToken())
- use VarDecl::getDescribedVarTemplate() and see if the described template has a name. I have no idea if this will work. If it works, it seems cleaner than the source location hacks or inspecting the raw tokens.

Alternatively, we can write this off as a cost of doing the conversion: kind of like the static const trait helpers, doing manual fixups is a relatively low cost and likely won't happen outside WTF.
Cc: dcheng@chromium.org
Owner: lukasza@chromium.org
Status: Started (was: Assigned)
getDescribedVarTemplate always returns null

I have a repro for the problem @ https://codereview.chromium.org/2246263002.

I am not sure if the fix I proposed in the same review is the right fix.  Tests (including the new tests) pass, but that might not mean much... :->
Status: Fixed (was: Started)
Status: Assigned (was: Fixed)
This is still happening :-(


Latest example is third_party/WebKit/Source/bindings/core/v8/V8StringResource.h where we got the following incorrect rewrite:

-CORE_EXPORT StringType v8StringToWebCoreString(v8::Local<v8::String>,
+CORE_EXPORT StringType V8StringToWebCoreString(v8::Local<v8::String>v8_string
Project Member

Comment 12 by bugdroid1@chromium.org, Dec 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/49c85e01a1cb4c9da1ce92d5792c87738eb26b70

commit 49c85e01a1cb4c9da1ce92d5792c87738eb26b70
Author: lukasza <lukasza@chromium.org>
Date: Thu Dec 22 00:44:01 2016

Tweaking lists of method names to treat in a special way.

BUG= 598141 

Review-Url: https://codereview.chromium.org/2595213002
Cr-Commit-Position: refs/heads/master@{#440284}

[modify] https://crrev.com/49c85e01a1cb4c9da1ce92d5792c87738eb26b70/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp

Status: Fixed (was: Assigned)
Comparing expected-vs-actual old text (from r440778) should help here.

Sign in to add a comment