New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Vacation back Sept
Closed: Jan 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature

Blocking:
issue 578344



Sign in to add a comment

HashMap::m_impl gets renamed wrong

Project Member Reported by danakj@chromium.org, Jan 23 2016

Issue description

It becomes "impl" instead of "impl_". Why? Who knows.
 

Comment 1 by danakj@chromium.org, Jan 23 2016

Blocking: chromium:578344

Comment 2 by danakj@chromium.org, Jan 23 2016

Summary: HashMap::m_impl gets renamed wrong (was: HashMap::m_impl get's renamed wrong)

Comment 3 by danakj@chromium.org, Jan 23 2016

Labels: -Type-Bug Type-Feature

Comment 4 by danakj@chromium.org, Jan 27 2016

Owner: danakj@chromium.org
Status: Started

Comment 5 by danakj@chromium.org, Jan 27 2016

Cc: thakis@chromium.org
This is something frightening going on.

If I do:

  const clang::RecordDecl* parent = decl.getParent();
  bool c = parent->isClass();
  if (c)
    name += '_';

Then it does the right thing.

If I do

  const clang::RecordDecl* parent = decl.getParent();
  if (parent->isClass())
    name += '_';

Then it does not. Tried building the tool with -fno-strict-aliasing to no effect.

Comment 6 by danakj@chromium.org, Jan 27 2016

Cc: h...@chromium.org
Owner: thakis@chromium.org
Status: Assigned
Um.. => assign to thakis maybe? I dunno where to go next with this.

FTR the change I was making is at https://code.google.com/p/chromium/codesearch#chromium/src/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp&l=114

Comment 7 by h...@chromium.org, Jan 27 2016

Dana: what OS was this on?

Comment 8 by thakis@chromium.org, Jan 27 2016

If you use the regular build setup, you build with gcc, not clang, right?

I do clang compiler bugs, but I don't do gcc compiler bugs :-P

I can verify if I can repro this though. (But not until I fixed my tooling crashes which are proving to be resilient :-( )

Comment 9 by danakj@chromium.org, Jan 27 2016

@hans: goobuntu linux

I just run update.py and ninja, so whatever it does. Maybe we could build this tool with the system clang?
No, we don't want to even hear about "system clang". There's only self-updating evergreen clang :-)

I think update.py downloads a prebuilt gcc4.8 from somewhere. We could change it download something else.

It's pretty scary that this goes wrong. What did you change to build the tool with -fno-strict-aliasing?
My update.py has these diffs

diff --git a/tools/clang/scripts/update.py b/tools/clang/scripts/update.py
index c2fc850..1820e60 100755
--- a/tools/clang/scripts/update.py
+++ b/tools/clang/scripts/update.py
@@ -538,6 +538,8 @@ def UpdateClang(args):
     cflags += ['-DLLVM_FORCE_HEAD_REVISION']
     cxxflags += ['-DLLVM_FORCE_HEAD_REVISION']
 
+  cflags += ['-g -O0 -fno-strict-aliasing']
+
   CreateChromeToolsShim()
 
   deployment_env = None


I don't think -O0 is working, probably overridden by -O2 somewhere.
Huh, that adds those as a single flag, no? Does

  cflags += [ '-g', '-O0', '-fno-strict-aliasing' ]

help? (but i guess it's all joined together in the end, so probably not)
Yeh, I saw that too, it shouldn't. I'm trying update.py with --bootstrap atm. dcheng suggested that might use clang to build the tool?
Yes.
Owner: danakj@chromium.org
Well, with --bootstrap, prints/asserts seem to verify that it's doing the right thing. And run_tool.py worked correctly on HashMap.

gdb can't find the source file so breakpoints don't work tho. You win some you lose some.

So I guess one answer here is that we must use --bootstrap. Any pointers on making gdb would be super lovely tho.

=> me to update documentation.
Project Member

Comment 16 by bugdroid1@chromium.org, Jan 28 2016

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

commit 30d0f8c92a47b357789076692ec2eeb571bf312e
Author: danakj <danakj@chromium.org>
Date: Thu Jan 28 00:26:33 2016

clang tools: Add --bootstrap to the documentation for update.py

This lets us build the clang tools with clang not gcc, which appears to
have bugs!

Also work around the gcc bug in one place we know it happens anyways,
just because.

R=dcheng
BUG= 580745 

Review URL: https://codereview.chromium.org/1647733002

Cr-Commit-Position: refs/heads/master@{#371933}

[modify] http://crrev.com/30d0f8c92a47b357789076692ec2eeb571bf312e/docs/clang_tool_refactoring.md
[modify] http://crrev.com/30d0f8c92a47b357789076692ec2eeb571bf312e/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp

Status: Fixed
"Fixed" by using --bootstrap, though gdb stops functioning for the tool and chrome builds now take ~infinity time. D:
Yeah, goma can't work if you have a locally built clang of course :-) you can remove third_party/CR_clang_revision and rerun update.py without args to get the remote-compatible clang binary again.

Not sure this should be marked fixed, we probably want to upload a GCC that doesn't randomly miscompile stuff. Our clang packages are all bootstrapped though, so they are fine.

Comment 19 by h...@chromium.org, Jan 28 2016

> Not sure this should be marked fixed, we probably want to upload a GCC that doesn't randomly miscompile stuff. Our clang packages are all bootstrapped though, so they are fine.

The

 bool c = parent->isClass();
  if (c)
    name += '_';

vs

  if (parent->isClass())
    name += '_';

seems like a strange thing to miscompile though, and we do that kind of thing everywhere in Clang, which we've been building with that gcc for a long time.

It would be nice to have a better understanding of what's going on before we decide that a new GCC is the fix.
How does one debug further?

I did verify by changing the code and watching it branch or not that the parent->getTagKind() is not returning a valid TagTypeKind (i tried branching on <= TTK_Enum and that also didn't branch).

It clearly does branch for some (or most?) things in the blink codebase, but for some reason not for the HashMap::m_impl one.

Comment 21 by h...@chromium.org, Jan 28 2016

My comment was mostly aimed at thakis, as I don't think we should rush to build a new GCC without understand things better. I don't have any concrete ideas for debugging this more.

Sign in to add a comment