Mangling & linkage of unnamed but instantiated nested classes

Hi John, (& cfe-dev)

I mentioned this in person last week & wanted to provide you with some
more details & ask for your opinion.

Backstory:

I originally came across this while trying to use
-Wunused-member-function & found it was flagging code like this:

struct foo {
  struct {
    void func() { ... } // warning that this is 'unused'
  } x;
};

This surprised me, so I looked around & found that this function has
"no linkage". This seemed strange (because I would expect to be able
to call the function from multiple TUs that included the header
defining 'foo', compare its address for equality, & such things) & it
looks like the Right Thing is for func() to have the same linkage as,
say, an inline member function of 'foo' would have.

The standard (3.5[basic.link]) seems to "miss" this case depending on
how you read it:

1) arguably p2, which says that "A name is said to have linkage when
it might denote the same ... function ... as a name introduced by a
declaration in another scope: - When a name has external linkage, the
entity it denotes can be referred to by names from scopes of other
translation units or from other scopes of the same translation unit"

2) p5: "... a member function ... has external linkage if the name of
the class has external linkage" (with an exception only for unnamed
classes (& enumerations) defined in class-scope typedef declarations
such that the class or enumeration has the dypedef name for linkage
purposes (7.1.3)) & there are no rules that seem to govern the linkage
of this unnamed class.
    p8 "Names not covered by these rules have no linkage"

The next step was also to look at the mangling compared to GCC. After
modifying the linkage of functions like this (with this change):

--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -496,8 +496,7 @@ static LinkageInfo getLVForClassMember(const
NamedDecl *D, bool OnlyTemplate) {
   if (!(isa<CXXMethodDecl>(D) ||
         isa<VarDecl>(D) ||
         isa<FieldDecl>(D) ||
- (isa<TagDecl>(D) &&
- (D->getDeclName() || cast<TagDecl>(D)->getTypedefNameForAnonDecl()))))
+ isa<TagDecl>(D)))
     return LinkageInfo::none();

   LinkageInfo LV;

(& Richard reckons we might be able to simplify that check - just to
eliminate some template cases that still seem to get filtered out by
it)

I caused one test to fail:
test/CodeGenCXX/template-anonymous-types.cpp. A modified version (to
better investigate GCC 4.7's mangling) looks like this:

struct S {
  enum { FOO = 42 };
  enum { BAR = 42 };
};

struct T {
  enum { FOO = 42 };
  enum { BAR = 42 };
};

template <typename T> struct X {
  T value;
  X(T t) : value(t) {}
  int f() { return value; }
};

template <typename T> int f(T t) {
  X<T> x(t);
  return x.f();
}

void test() {
  (void)f(S::FOO);
  (void)f(S::BAR);
  (void)f(T::FOO);
  (void)f(T::BAR);
}

& with my change we get the right linkage for the instantiations of
'f' (linkonce_odr instead of internal) but the mangling is still
inconsistent with GCC at least. Clang mangles these 4 'f's as:

_Z1fIN1S3$_0EEiT_
_Z1fIN1S3$_1EEiT_
_Z1fIN1T3$_2EEiT_
_Z1fIN1T3$_3EEiT_

GCC 4.7 mangles them as:

_Z1fIN1SUt_EEiT_
_Z1fIN1SUt0_EEiT_
_Z1fIN1TUt_EEiT_
_Z1fIN1TUt0_EEiT_

I don't think we have any class-specific unnamed nested type counter
that would implement that Ut_, Ut0_, ... mangling scheme, though I can
imagine where one might be added (I'm not very familiar with IRGen
though, so I'll certainly be happy to have any pointers about how that
could/should be implemented).

What do you think? Could someone (John, presumably) consult the ABI
oracles & explain what we should be doing here?

- David

Probably we would want to record and remember this in the AST.

This schema is indeed the one blessed by the ABI.
   <unnamed-type-name> ::= Ut [ <nonnegative number> ] _
But as the ABI also notes:
  The mangling of such unnamed types defined in namespace scope is generally unspecified because they do not have to match across translation units. An implementation must only ensure that naming collisions are avoided.

The interesting question is what linkage GCC actually gives these symbols.

John.

GCC 4.7 mangles them as:

Z1fIN1SUt_EEiT
Z1fIN1SUt0_EEiT
Z1fIN1TUt_EEiT
Z1fIN1TUt0_EEiT

I don’t think we have any class-specific unnamed nested type counter
that would implement that Ut_, Ut0_, … mangling scheme, though I can
imagine where one might be added (I’m not very familiar with IRGen
though, so I’ll certainly be happy to have any pointers about how that
could/should be implemented).

Probably we would want to record and remember this in the AST.

This schema is indeed the one blessed by the ABI.
::= Ut [ ] _
But as the ABI also notes:
The mangling of such unnamed types defined in namespace scope is generally unspecified because they do not have to match across translation units. An implementation must only ensure that naming collisions are avoided.

But David’s original example does not have an unnamed type in namespace scope.

GCC 4.7 mangles them as:

_Z1fIN1SUt_EEiT_
_Z1fIN1SUt0_EEiT_
_Z1fIN1TUt_EEiT_
_Z1fIN1TUt0_EEiT_

I don't think we have any class-specific unnamed nested type counter
that would implement that Ut_, Ut0_, ... mangling scheme, though I can
imagine where one might be added (I'm not very familiar with IRGen
though, so I'll certainly be happy to have any pointers about how that
could/should be implemented).

Probably we would want to record and remember this in the AST.

This schema is indeed the one blessed by the ABI.
   <unnamed-type-name> ::= Ut [ <nonnegative number> ] _
But as the ABI also notes:
  The mangling of such unnamed types defined in namespace scope is generally unspecified because they do not have to match across translation units. An implementation must only ensure that naming collisions are avoided.

Hmm, that sounds you're saying we're getting this mangling wrong for
namespace scope cases of similar constructs - but that in /that/ case
we don't have to worry about numbering. I assume the implication is
that we do have to worry about numbering in the external-linkage class
scope case I'm talking about?

The interesting question is what linkage GCC actually gives these symbols.

Right, that's part of the puzzle. Not knowing much about how linkage
is represented in assembly I've just compared this fairly
mechanically.

Clang ToT emits something like this:

  .type _Z1fIN1S3$_0EEiT_,@function

Clang with my linkage change (that changes the linkage of the nested
type from "no linkage" to (the linkage of the outer class)
"linkonce_odr") emits something like this:

  .section
".text._Z1fIN1S3$_0EEiT_","axG",@progbits,_Z1fIN1S3$_0EEiT_,comdat
  .weak _Z1fIN1S3$_0EEiT_
  .align 16, 0x90
  .type _Z1fIN1S3$_0EEiT_,@function

& GCC emits something similar, sans the alignment for whatever that's worth:

  .section .text._Z1fIN1SUt_EEiT_,"axG",@progbits,_Z1fIN1SUt_EEiT_,comdat
  .weak _Z1fIN1SUt_EEiT_
  .type _Z1fIN1SUt_EEiT_, @function

So I'll assume GCC is emitting these with linkonce_odr. (sorry, I'm
not up on the actual ABI terminology for these different linkage
classes, so I'm using the LLVM one)

Hi John, (& cfe-dev)

I mentioned this in person last week & wanted to provide you with some
more details & ask for your opinion.

Backstory:

I originally came across this while trying to use
-Wunused-member-function & found it was flagging code like this:

struct foo {
  struct {
    void func() { ... } // warning that this is 'unused'
  } x;
};

This surprised me, so I looked around & found that this function has
"no linkage". This seemed strange (because I would expect to be able
to call the function from multiple TUs that included the header
defining 'foo', compare its address for equality, & such things) & it
looks like the Right Thing is for func() to have the same linkage as,
say, an inline member function of 'foo' would have.

The standard (3.5[basic.link]) seems to "miss" this case depending on
how you read it:

1) arguably p2, which says that "A name is said to have linkage when
it might denote the same ... function ... as a name introduced by a
declaration in another scope: - When a name has external linkage, the
entity it denotes can be referred to by names from scopes of other
translation units or from other scopes of the same translation unit"

2) p5: "... a member function ... has external linkage if the name of
the class has external linkage" (with an exception only for unnamed
classes (& enumerations) defined in class-scope typedef declarations
such that the class or enumeration has the dypedef name for linkage
purposes (7.1.3)) & there are no rules that seem to govern the linkage
of this unnamed class.
    p8 "Names not covered by these rules have no linkage"

[basic.link]p8: "A type is said to have linkage if and only if [...]
it is an unnamed class or enumeration member of a class with linkage".

I'm going to assume it's an oversight in the standard that the members
of such an unnamed class don't have linkage.

The next step was also to look at the mangling compared to GCC. After
modifying the linkage of functions like this (with this change):

--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -496,8 +496,7 @@ static LinkageInfo getLVForClassMember(const
NamedDecl *D, bool OnlyTemplate) {
   if (!(isa<CXXMethodDecl>(D) ||
         isa<VarDecl>(D) ||
         isa<FieldDecl>(D) ||
- (isa<TagDecl>(D) &&
- (D->getDeclName() || cast<TagDecl>(D)->getTypedefNameForAnonDecl()))))
+ isa<TagDecl>(D)))
     return LinkageInfo::none();

   LinkageInfo LV;

(& Richard reckons we might be able to simplify that check - just to
eliminate some template cases that still seem to get filtered out by
it)

This looks like a step in the right direction.

I caused one test to fail:
test/CodeGenCXX/template-anonymous-types.cpp. A modified version (to
better investigate GCC 4.7's mangling) looks like this:

struct S {
  enum { FOO = 42 };
  enum { BAR = 42 };
};

struct T {
  enum { FOO = 42 };
  enum { BAR = 42 };
};

template <typename T> struct X {
  T value;
  X(T t) : value(t) {}
  int f() { return value; }
};

template <typename T> int f(T t) {
  X<T> x(t);
  return x.f();
}

void test() {
  (void)f(S::FOO);
  (void)f(S::BAR);
  (void)f(T::FOO);
  (void)f(T::BAR);
}

& with my change we get the right linkage for the instantiations of
'f' (linkonce_odr instead of internal) but the mangling is still
inconsistent with GCC at least. Clang mangles these 4 'f's as:

_Z1fIN1S3$_0EEiT_
_Z1fIN1S3$_1EEiT_
_Z1fIN1T3$_2EEiT_
_Z1fIN1T3$_3EEiT_

GCC 4.7 mangles them as:

_Z1fIN1SUt_EEiT_
_Z1fIN1SUt0_EEiT_
_Z1fIN1TUt_EEiT_
_Z1fIN1TUt0_EEiT_

I don't think we have any class-specific unnamed nested type counter
that would implement that Ut_, Ut0_, ... mangling scheme, though I can
imagine where one might be added (I'm not very familiar with IRGen
though, so I'll certainly be happy to have any pointers about how that
could/should be implemented).

We have a similar scheme already implemented for lambdas; look at
getLambdaManglingNumber() etc.

-Eli

For the following:

struct S {
  struct {
    int f() {}
    struct {} n;
  } s;
};
int k1 = S().s.f();
decltype(S().s.n) n1;

struct {
  int g() {}
  struct {} n;
} s;
int k2 = s.g();
decltype(s.n) n2;

Run through g++-4.7 -S and nm, we get:

0000000000000000 t _ZN1SUt_1fEv
000000000000000a t _ZN3._21gEv
0000000000000004 B n1
000000000000000d b n2

So... it looks like g++ believes that S::<anon>::n's type has external
linkage (because n1 has external linkage), and s.n's type has internal
linkage (because n2 has internal linkage). It also looks like g++
believes both S::<anon>::f and <anon>::g don't need to be linked
across TUs, in violation of [basic.def.odr]p6's requirement that "the
program shall behave as if there were a single definition of
[S::<anon>]".

The rules also seem to miss anonymous classes declared within inline
functions, for which we also pick an incorrect mangled name and
linkage:

inline int f() {
  struct {
    int f() { return 0; }
  } s;
  return s.f();
}
int k = f();

Clang emits:

0000000000000000 t _ZZ1fvEN3$_01fEv

g++ emits:

0000000000000000 W _ZZ1fvENUt_1fEv

Thanks for the pointer. Following the lines there I've had a first
blush at this (see attached).

I've not addressed some of the issues Richard brought up but I'd be
happy to take the time to generalize this to handle.

- David

mangle.diff (7.67 KB)

+ if (!Tag->getName().empty() || Tag->getTypedefNameForAnonDecl() ||
+ !isa<CXXRecordDecl>(Tag->getParent()))
+ return -2;

Hi John, (& cfe-dev)

I mentioned this in person last week & wanted to provide you with some
more details & ask for your opinion.

Backstory:

I originally came across this while trying to use
-Wunused-member-function & found it was flagging code like this:

struct foo {
  struct {
    void func() { ... } // warning that this is 'unused'
  } x;
};

This surprised me, so I looked around & found that this function has
"no linkage". This seemed strange (because I would expect to be able
to call the function from multiple TUs that included the header
defining 'foo', compare its address for equality, & such things) & it
looks like the Right Thing is for func() to have the same linkage as,
say, an inline member function of 'foo' would have.

The standard (3.5[basic.link]) seems to "miss" this case depending on
how you read it:

1) arguably p2, which says that "A name is said to have linkage when
it might denote the same ... function ... as a name introduced by a
declaration in another scope: - When a name has external linkage, the
entity it denotes can be referred to by names from scopes of other
translation units or from other scopes of the same translation unit"

2) p5: "... a member function ... has external linkage if the name of
the class has external linkage" (with an exception only for unnamed
classes (& enumerations) defined in class-scope typedef declarations
such that the class or enumeration has the dypedef name for linkage
purposes (7.1.3)) & there are no rules that seem to govern the linkage
of this unnamed class.
    p8 "Names not covered by these rules have no linkage"

[basic.link]p8: "A type is said to have linkage if and only if [...]
it is an unnamed class or enumeration member of a class with linkage".

I'm going to assume it's an oversight in the standard that the members
of such an unnamed class don't have linkage.

The next step was also to look at the mangling compared to GCC. After
modifying the linkage of functions like this (with this change):

--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -496,8 +496,7 @@ static LinkageInfo getLVForClassMember(const
NamedDecl *D, bool OnlyTemplate) {
   if (!(isa<CXXMethodDecl>(D) ||
         isa<VarDecl>(D) ||
         isa<FieldDecl>(D) ||
- (isa<TagDecl>(D) &&
- (D->getDeclName() || cast<TagDecl>(D)->getTypedefNameForAnonDecl()))))
+ isa<TagDecl>(D)))
     return LinkageInfo::none();

   LinkageInfo LV;

(& Richard reckons we might be able to simplify that check - just to
eliminate some template cases that still seem to get filtered out by
it)

This looks like a step in the right direction.

I caused one test to fail:
test/CodeGenCXX/template-anonymous-types.cpp. A modified version (to
better investigate GCC 4.7's mangling) looks like this:

struct S {
  enum { FOO = 42 };
  enum { BAR = 42 };
};

struct T {
  enum { FOO = 42 };
  enum { BAR = 42 };
};

template <typename T> struct X {
  T value;
  X(T t) : value(t) {}
  int f() { return value; }
};

template <typename T> int f(T t) {
  X<T> x(t);
  return x.f();
}

void test() {
  (void)f(S::FOO);
  (void)f(S::BAR);
  (void)f(T::FOO);
  (void)f(T::BAR);
}

& with my change we get the right linkage for the instantiations of
'f' (linkonce_odr instead of internal) but the mangling is still
inconsistent with GCC at least. Clang mangles these 4 'f's as:

_Z1fIN1S3$_0EEiT_
_Z1fIN1S3$_1EEiT_
_Z1fIN1T3$_2EEiT_
_Z1fIN1T3$_3EEiT_

GCC 4.7 mangles them as:

_Z1fIN1SUt_EEiT_
_Z1fIN1SUt0_EEiT_
_Z1fIN1TUt_EEiT_
_Z1fIN1TUt0_EEiT_

I don't think we have any class-specific unnamed nested type counter
that would implement that Ut_, Ut0_, ... mangling scheme, though I can
imagine where one might be added (I'm not very familiar with IRGen
though, so I'll certainly be happy to have any pointers about how that
could/should be implemented).

We have a similar scheme already implemented for lambdas; look at
getLambdaManglingNumber() etc.

Thanks for the pointer. Following the lines there I've had a first
blush at this (see attached).

I've not addressed some of the issues Richard brought up but I'd be
happy to take the time to generalize this to handle.

Spent a little time trying to generalize this to handle the function
local case but it was taking a bit too much of my time so I figured
I'd at least get this case handled (which addresses my issue of
-Wunused-function & generally improves the world a little bit at
least).

+ if (!Tag->getName().empty() || Tag->getTypedefNameForAnonDecl() ||
+ !isa<CXXRecordDecl>(Tag->getParent()))
+ return -2;
+
+ std::pair<llvm::DenseMap<const DeclContext *, int>::iterator, bool>
P = UnnamedMangleContexts.insert(std::make_pair(Tag->getParent(),
-1));

getParent() doesn't return a canonical DeclContext.

I'm not entirely sure what bugs this might cause (test cases welcome)
but I believe I've addressed this in the newly attached patch.

Also, 80 columns.

Right, sorry, I was mostly looking to see if I was generally on the
right track. Fixed, though.

Also, we probably don't want to generate numbers for a decl unless
the mangling is actually externally visible.

Done.

--- include/clang/AST/Decl.h
+++ include/clang/AST/Decl.h
@@ -2486,6 +2486,8 @@ private:
   /// otherwise, it is a null (TypedefNameDecl) pointer.
   llvm::PointerUnion<TypedefNameDecl*, ExtInfo*> TypedefNameDeclOrQualifier;

+ int UnnamedManglingNumber;
+
   bool hasExtInfo() const { return TypedefNameDeclOrQualifier.is<ExtInfo*>(); }
   ExtInfo *getExtInfo() { return TypedefNameDeclOrQualifier.get<ExtInfo*>(); }
   const ExtInfo *getExtInfo() const {

We don't really want to add an extra member to DeclContext just to
handle an obscure edge case.

Used a separate side table in the ASTContext to keep the numbers. (& I
changed the numbers to be simpler - counting from zero instead of -1,
then doing an offset to compute the actual mangle number. Let me know
if you'd prefer it to work the other way)

- David

mangle.diff (6.78 KB)

(now with the actually up-to-date patch)

mangle.diff (6.85 KB)

Comments below.

Hi John, (& cfe-dev)

I mentioned this in person last week & wanted to provide you with some
more details & ask for your opinion.

Backstory:

I originally came across this while trying to use
-Wunused-member-function & found it was flagging code like this:

struct foo {
  struct {
    void func() { ... } // warning that this is 'unused'
  } x;
};

This surprised me, so I looked around & found that this function has
"no linkage". This seemed strange (because I would expect to be able
to call the function from multiple TUs that included the header
defining 'foo', compare its address for equality, & such things) & it
looks like the Right Thing is for func() to have the same linkage as,
say, an inline member function of 'foo' would have.

The standard (3.5[basic.link]) seems to "miss" this case depending on
how you read it:

1) arguably p2, which says that "A name is said to have linkage when
it might denote the same ... function ... as a name introduced by a
declaration in another scope: - When a name has external linkage, the
entity it denotes can be referred to by names from scopes of other
translation units or from other scopes of the same translation unit"

2) p5: "... a member function ... has external linkage if the name of
the class has external linkage" (with an exception only for unnamed
classes (& enumerations) defined in class-scope typedef declarations
such that the class or enumeration has the dypedef name for linkage
purposes (7.1.3)) & there are no rules that seem to govern the linkage
of this unnamed class.
    p8 "Names not covered by these rules have no linkage"

[basic.link]p8: "A type is said to have linkage if and only if [...]
it is an unnamed class or enumeration member of a class with linkage".

I'm going to assume it's an oversight in the standard that the members
of such an unnamed class don't have linkage.

The next step was also to look at the mangling compared to GCC. After
modifying the linkage of functions like this (with this change):

--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -496,8 +496,7 @@ static LinkageInfo getLVForClassMember(const
NamedDecl *D, bool OnlyTemplate) {
   if (!(isa<CXXMethodDecl>(D) ||
         isa<VarDecl>(D) ||
         isa<FieldDecl>(D) ||
- (isa<TagDecl>(D) &&
- (D->getDeclName() || cast<TagDecl>(D)->getTypedefNameForAnonDecl()))))
+ isa<TagDecl>(D)))
     return LinkageInfo::none();

   LinkageInfo LV;

(& Richard reckons we might be able to simplify that check - just to
eliminate some template cases that still seem to get filtered out by
it)

This looks like a step in the right direction.

I caused one test to fail:
test/CodeGenCXX/template-anonymous-types.cpp. A modified version (to
better investigate GCC 4.7's mangling) looks like this:

struct S {
  enum { FOO = 42 };
  enum { BAR = 42 };
};

struct T {
  enum { FOO = 42 };
  enum { BAR = 42 };
};

template <typename T> struct X {
  T value;
  X(T t) : value(t) {}
  int f() { return value; }
};

template <typename T> int f(T t) {
  X<T> x(t);
  return x.f();
}

void test() {
  (void)f(S::FOO);
  (void)f(S::BAR);
  (void)f(T::FOO);
  (void)f(T::BAR);
}

& with my change we get the right linkage for the instantiations of
'f' (linkonce_odr instead of internal) but the mangling is still
inconsistent with GCC at least. Clang mangles these 4 'f's as:

_Z1fIN1S3$_0EEiT_
_Z1fIN1S3$_1EEiT_
_Z1fIN1T3$_2EEiT_
_Z1fIN1T3$_3EEiT_

GCC 4.7 mangles them as:

_Z1fIN1SUt_EEiT_
_Z1fIN1SUt0_EEiT_
_Z1fIN1TUt_EEiT_
_Z1fIN1TUt0_EEiT_

I don't think we have any class-specific unnamed nested type counter
that would implement that Ut_, Ut0_, ... mangling scheme, though I can
imagine where one might be added (I'm not very familiar with IRGen
though, so I'll certainly be happy to have any pointers about how that
could/should be implemented).

We have a similar scheme already implemented for lambdas; look at
getLambdaManglingNumber() etc.

Thanks for the pointer. Following the lines there I've had a first
blush at this (see attached).

I've not addressed some of the issues Richard brought up but I'd be
happy to take the time to generalize this to handle.

Spent a little time trying to generalize this to handle the function
local case but it was taking a bit too much of my time so I figured
I'd at least get this case handled (which addresses my issue of
-Wunused-function & generally improves the world a little bit at
least).

+ if (!Tag->getName().empty() || Tag->getTypedefNameForAnonDecl() ||
+ !isa<CXXRecordDecl>(Tag->getParent()))
+ return -2;
+
+ std::pair<llvm::DenseMap<const DeclContext *, int>::iterator, bool>
P = UnnamedMangleContexts.insert(std::make_pair(Tag->getParent(),
-1));

getParent() doesn't return a canonical DeclContext.

I'm not entirely sure what bugs this might cause (test cases welcome)
but I believe I've addressed this in the newly attached patch.

It might not matter here, because all decls contained in a RecordDecl
generally have the same parent... this has a bigger impact for
NamespaceDecl, for example, because there are two different Decls
which are semantically the same context.

Also, 80 columns.

Right, sorry, I was mostly looking to see if I was generally on the
right track. Fixed, though.

Also, we probably don't want to generate numbers for a decl unless
the mangling is actually externally visible.

Done.

--- include/clang/AST/Decl.h
+++ include/clang/AST/Decl.h
@@ -2486,6 +2486,8 @@ private:
   /// otherwise, it is a null (TypedefNameDecl) pointer.
   llvm::PointerUnion<TypedefNameDecl*, ExtInfo*> TypedefNameDeclOrQualifier;

+ int UnnamedManglingNumber;
+
   bool hasExtInfo() const { return TypedefNameDeclOrQualifier.is<ExtInfo*>(); }
   ExtInfo *getExtInfo() { return TypedefNameDeclOrQualifier.get<ExtInfo*>(); }
   const ExtInfo *getExtInfo() const {

We don't really want to add an extra member to DeclContext just to
handle an obscure edge case.

Used a separate side table in the ASTContext to keep the numbers. (& I
changed the numbers to be simpler - counting from zero instead of -1,
then doing an offset to compute the actual mangle number. Let me know
if you'd prefer it to work the other way)

(now with the actually up-to-date patch)

@@ -2582,8 +2581,9 @@ void
TagDecl::setTypedefNameForAnonDecl(TypedefNameDecl *TDD) {
void TagDecl::startDefinition() {
   IsBeingDefined = true;

- if (isa<CXXRecordDecl>(this)) {
- CXXRecordDecl *D = cast<CXXRecordDecl>(this);
+ getASTContext().addUnnamedTag(this);

Will getTypedefNameForAnonDecl actually return the correct result
here? (I'd like to see a test to make sure that "typedef struct {} x"
doesn't cause us to increment the mangling number, assuming that's
consistent with gcc.)

-Eli

Comments below.

Hi John, (& cfe-dev)

I mentioned this in person last week & wanted to provide you with some
more details & ask for your opinion.

Backstory:

I originally came across this while trying to use
-Wunused-member-function & found it was flagging code like this:

struct foo {
  struct {
    void func() { ... } // warning that this is 'unused'
  } x;
};

This surprised me, so I looked around & found that this function has
"no linkage". This seemed strange (because I would expect to be able
to call the function from multiple TUs that included the header
defining 'foo', compare its address for equality, & such things) & it
looks like the Right Thing is for func() to have the same linkage as,
say, an inline member function of 'foo' would have.

The standard (3.5[basic.link]) seems to "miss" this case depending on
how you read it:

1) arguably p2, which says that "A name is said to have linkage when
it might denote the same ... function ... as a name introduced by a
declaration in another scope: - When a name has external linkage, the
entity it denotes can be referred to by names from scopes of other
translation units or from other scopes of the same translation unit"

2) p5: "... a member function ... has external linkage if the name of
the class has external linkage" (with an exception only for unnamed
classes (& enumerations) defined in class-scope typedef declarations
such that the class or enumeration has the dypedef name for linkage
purposes (7.1.3)) & there are no rules that seem to govern the linkage
of this unnamed class.
    p8 "Names not covered by these rules have no linkage"

[basic.link]p8: "A type is said to have linkage if and only if [...]
it is an unnamed class or enumeration member of a class with linkage".

I'm going to assume it's an oversight in the standard that the members
of such an unnamed class don't have linkage.

The next step was also to look at the mangling compared to GCC. After
modifying the linkage of functions like this (with this change):

--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -496,8 +496,7 @@ static LinkageInfo getLVForClassMember(const
NamedDecl *D, bool OnlyTemplate) {
   if (!(isa<CXXMethodDecl>(D) ||
         isa<VarDecl>(D) ||
         isa<FieldDecl>(D) ||
- (isa<TagDecl>(D) &&
- (D->getDeclName() || cast<TagDecl>(D)->getTypedefNameForAnonDecl()))))
+ isa<TagDecl>(D)))
     return LinkageInfo::none();

   LinkageInfo LV;

(& Richard reckons we might be able to simplify that check - just to
eliminate some template cases that still seem to get filtered out by
it)

This looks like a step in the right direction.

I caused one test to fail:
test/CodeGenCXX/template-anonymous-types.cpp. A modified version (to
better investigate GCC 4.7's mangling) looks like this:

struct S {
  enum { FOO = 42 };
  enum { BAR = 42 };
};

struct T {
  enum { FOO = 42 };
  enum { BAR = 42 };
};

template <typename T> struct X {
  T value;
  X(T t) : value(t) {}
  int f() { return value; }
};

template <typename T> int f(T t) {
  X<T> x(t);
  return x.f();
}

void test() {
  (void)f(S::FOO);
  (void)f(S::BAR);
  (void)f(T::FOO);
  (void)f(T::BAR);
}

& with my change we get the right linkage for the instantiations of
'f' (linkonce_odr instead of internal) but the mangling is still
inconsistent with GCC at least. Clang mangles these 4 'f's as:

_Z1fIN1S3$_0EEiT_
_Z1fIN1S3$_1EEiT_
_Z1fIN1T3$_2EEiT_
_Z1fIN1T3$_3EEiT_

GCC 4.7 mangles them as:

_Z1fIN1SUt_EEiT_
_Z1fIN1SUt0_EEiT_
_Z1fIN1TUt_EEiT_
_Z1fIN1TUt0_EEiT_

I don't think we have any class-specific unnamed nested type counter
that would implement that Ut_, Ut0_, ... mangling scheme, though I can
imagine where one might be added (I'm not very familiar with IRGen
though, so I'll certainly be happy to have any pointers about how that
could/should be implemented).

We have a similar scheme already implemented for lambdas; look at
getLambdaManglingNumber() etc.

Thanks for the pointer. Following the lines there I've had a first
blush at this (see attached).

I've not addressed some of the issues Richard brought up but I'd be
happy to take the time to generalize this to handle.

Spent a little time trying to generalize this to handle the function
local case but it was taking a bit too much of my time so I figured
I'd at least get this case handled (which addresses my issue of
-Wunused-function & generally improves the world a little bit at
least).

+ if (!Tag->getName().empty() || Tag->getTypedefNameForAnonDecl() ||
+ !isa<CXXRecordDecl>(Tag->getParent()))
+ return -2;
+
+ std::pair<llvm::DenseMap<const DeclContext *, int>::iterator, bool>
P = UnnamedMangleContexts.insert(std::make_pair(Tag->getParent(),
-1));

getParent() doesn't return a canonical DeclContext.

I'm not entirely sure what bugs this might cause (test cases welcome)
but I believe I've addressed this in the newly attached patch.

It might not matter here, because all decls contained in a RecordDecl
generally have the same parent... this has a bigger impact for
NamespaceDecl, for example, because there are two different Decls
which are semantically the same context.

Thanks for the explanation.

Also, 80 columns.

Right, sorry, I was mostly looking to see if I was generally on the
right track. Fixed, though.

Also, we probably don't want to generate numbers for a decl unless
the mangling is actually externally visible.

Done.

--- include/clang/AST/Decl.h
+++ include/clang/AST/Decl.h
@@ -2486,6 +2486,8 @@ private:
   /// otherwise, it is a null (TypedefNameDecl) pointer.
   llvm::PointerUnion<TypedefNameDecl*, ExtInfo*> TypedefNameDeclOrQualifier;

+ int UnnamedManglingNumber;
+
   bool hasExtInfo() const { return TypedefNameDeclOrQualifier.is<ExtInfo*>(); }
   ExtInfo *getExtInfo() { return TypedefNameDeclOrQualifier.get<ExtInfo*>(); }
   const ExtInfo *getExtInfo() const {

We don't really want to add an extra member to DeclContext just to
handle an obscure edge case.

Used a separate side table in the ASTContext to keep the numbers. (& I
changed the numbers to be simpler - counting from zero instead of -1,
then doing an offset to compute the actual mangle number. Let me know
if you'd prefer it to work the other way)

(now with the actually up-to-date patch)

@@ -2582,8 +2581,9 @@ void
TagDecl::setTypedefNameForAnonDecl(TypedefNameDecl *TDD) {
void TagDecl::startDefinition() {
   IsBeingDefined = true;

- if (isa<CXXRecordDecl>(this)) {
- CXXRecordDecl *D = cast<CXXRecordDecl>(this);
+ getASTContext().addUnnamedTag(this);

Will getTypedefNameForAnonDecl actually return the correct result
here?

Seems not - thanks for the catch.

(I'd like to see a test to make sure that "typedef struct {} x"
doesn't cause us to increment the mangling number, assuming that's
consistent with gcc.)

Test case added & then I had to restructure the code to address this.
Since we don't know which way a type will be mangled until after we've
parsed all the declarators in the group (eg: typedef struct{} *x, *y,
z; - only when we see 'z' do we know that we have a linkage name for
the struct, if we only have x and y, then we don't have such a name &
we need to use the UtX_ naming) I had to inject the logic into
FinalizeDeclaratorGroup (or somewhere near there - perhaps there's a
better spot). This means we miss the original test case failure of:
struct foo { enum { X }; }; since there is no declarator group in tht
instance, just a standalone decl. So I added this to the
ParsedFreeStandingDeclSpec as well.

If there's some more appropriate common point to put this logic, I'm
all ears - but it seems to me there's no common point in Sema for
these two cases so it's either duplicated or could be non-duplicated
if it were up in the Parser (or had a new Sema entry point to call
from there).

Open to ideas - thanks again for the help/review/pointers.

- David

+patch

mangle.diff (8.98 KB)

+patch

Comments below.

Hi John, (& cfe-dev)

I mentioned this in person last week & wanted to provide you with some
more details & ask for your opinion.

Backstory:

I originally came across this while trying to use
-Wunused-member-function & found it was flagging code like this:

struct foo {
  struct {
    void func() { ... } // warning that this is 'unused'
  } x;
};

This surprised me, so I looked around & found that this function has
"no linkage". This seemed strange (because I would expect to be able
to call the function from multiple TUs that included the header
defining 'foo', compare its address for equality, & such things) & it
looks like the Right Thing is for func() to have the same linkage as,
say, an inline member function of 'foo' would have.

The standard (3.5[basic.link]) seems to "miss" this case depending on
how you read it:

1) arguably p2, which says that "A name is said to have linkage when
it might denote the same ... function ... as a name introduced by a
declaration in another scope: - When a name has external linkage, the
entity it denotes can be referred to by names from scopes of other
translation units or from other scopes of the same translation unit"

2) p5: "... a member function ... has external linkage if the name of
the class has external linkage" (with an exception only for unnamed
classes (& enumerations) defined in class-scope typedef declarations
such that the class or enumeration has the dypedef name for linkage
purposes (7.1.3)) & there are no rules that seem to govern the linkage
of this unnamed class.
    p8 "Names not covered by these rules have no linkage"

[basic.link]p8: "A type is said to have linkage if and only if [...]
it is an unnamed class or enumeration member of a class with linkage".

I'm going to assume it's an oversight in the standard that the members
of such an unnamed class don't have linkage.

The next step was also to look at the mangling compared to GCC. After
modifying the linkage of functions like this (with this change):

--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -496,8 +496,7 @@ static LinkageInfo getLVForClassMember(const
NamedDecl *D, bool OnlyTemplate) {
   if (!(isa<CXXMethodDecl>(D) ||
         isa<VarDecl>(D) ||
         isa<FieldDecl>(D) ||
- (isa<TagDecl>(D) &&
- (D->getDeclName() || cast<TagDecl>(D)->getTypedefNameForAnonDecl()))))
+ isa<TagDecl>(D)))
     return LinkageInfo::none();

   LinkageInfo LV;

(& Richard reckons we might be able to simplify that check - just to
eliminate some template cases that still seem to get filtered out by
it)

This looks like a step in the right direction.

I caused one test to fail:
test/CodeGenCXX/template-anonymous-types.cpp. A modified version (to
better investigate GCC 4.7's mangling) looks like this:

struct S {
  enum { FOO = 42 };
  enum { BAR = 42 };
};

struct T {
  enum { FOO = 42 };
  enum { BAR = 42 };
};

template <typename T> struct X {
  T value;
  X(T t) : value(t) {}
  int f() { return value; }
};

template <typename T> int f(T t) {
  X<T> x(t);
  return x.f();
}

void test() {
  (void)f(S::FOO);
  (void)f(S::BAR);
  (void)f(T::FOO);
  (void)f(T::BAR);
}

& with my change we get the right linkage for the instantiations of
'f' (linkonce_odr instead of internal) but the mangling is still
inconsistent with GCC at least. Clang mangles these 4 'f's as:

_Z1fIN1S3$_0EEiT_
_Z1fIN1S3$_1EEiT_
_Z1fIN1T3$_2EEiT_
_Z1fIN1T3$_3EEiT_

GCC 4.7 mangles them as:

_Z1fIN1SUt_EEiT_
_Z1fIN1SUt0_EEiT_
_Z1fIN1TUt_EEiT_
_Z1fIN1TUt0_EEiT_

I don't think we have any class-specific unnamed nested type counter
that would implement that Ut_, Ut0_, ... mangling scheme, though I can
imagine where one might be added (I'm not very familiar with IRGen
though, so I'll certainly be happy to have any pointers about how that
could/should be implemented).

We have a similar scheme already implemented for lambdas; look at
getLambdaManglingNumber() etc.

Thanks for the pointer. Following the lines there I've had a first
blush at this (see attached).

I've not addressed some of the issues Richard brought up but I'd be
happy to take the time to generalize this to handle.

Spent a little time trying to generalize this to handle the function
local case but it was taking a bit too much of my time so I figured
I'd at least get this case handled (which addresses my issue of
-Wunused-function & generally improves the world a little bit at
least).

+ if (!Tag->getName().empty() || Tag->getTypedefNameForAnonDecl() ||
+ !isa<CXXRecordDecl>(Tag->getParent()))
+ return -2;
+
+ std::pair<llvm::DenseMap<const DeclContext *, int>::iterator, bool>
P = UnnamedMangleContexts.insert(std::make_pair(Tag->getParent(),
-1));

getParent() doesn't return a canonical DeclContext.

I'm not entirely sure what bugs this might cause (test cases welcome)
but I believe I've addressed this in the newly attached patch.

It might not matter here, because all decls contained in a RecordDecl
generally have the same parent... this has a bigger impact for
NamespaceDecl, for example, because there are two different Decls
which are semantically the same context.

Thanks for the explanation.

Also, 80 columns.

Right, sorry, I was mostly looking to see if I was generally on the
right track. Fixed, though.

Also, we probably don't want to generate numbers for a decl unless
the mangling is actually externally visible.

Done.

--- include/clang/AST/Decl.h
+++ include/clang/AST/Decl.h
@@ -2486,6 +2486,8 @@ private:
   /// otherwise, it is a null (TypedefNameDecl) pointer.
   llvm::PointerUnion<TypedefNameDecl*, ExtInfo*> TypedefNameDeclOrQualifier;

+ int UnnamedManglingNumber;
+
   bool hasExtInfo() const { return TypedefNameDeclOrQualifier.is<ExtInfo*>(); }
   ExtInfo *getExtInfo() { return TypedefNameDeclOrQualifier.get<ExtInfo*>(); }
   const ExtInfo *getExtInfo() const {

We don't really want to add an extra member to DeclContext just to
handle an obscure edge case.

Used a separate side table in the ASTContext to keep the numbers. (& I
changed the numbers to be simpler - counting from zero instead of -1,
then doing an offset to compute the actual mangle number. Let me know
if you'd prefer it to work the other way)

(now with the actually up-to-date patch)

@@ -2582,8 +2581,9 @@ void
TagDecl::setTypedefNameForAnonDecl(TypedefNameDecl *TDD) {
void TagDecl::startDefinition() {
   IsBeingDefined = true;

- if (isa<CXXRecordDecl>(this)) {
- CXXRecordDecl *D = cast<CXXRecordDecl>(this);
+ getASTContext().addUnnamedTag(this);

Will getTypedefNameForAnonDecl actually return the correct result
here?

Seems not - thanks for the catch.

(I'd like to see a test to make sure that "typedef struct {} x"
doesn't cause us to increment the mangling number, assuming that's
consistent with gcc.)

Test case added & then I had to restructure the code to address this.
Since we don't know which way a type will be mangled until after we've
parsed all the declarators in the group (eg: typedef struct{} *x, *y,
z; - only when we see 'z' do we know that we have a linkage name for
the struct, if we only have x and y, then we don't have such a name &
we need to use the UtX_ naming) I had to inject the logic into
FinalizeDeclaratorGroup (or somewhere near there - perhaps there's a
better spot). This means we miss the original test case failure of:
struct foo { enum { X }; }; since there is no declarator group in tht
instance, just a standalone decl. So I added this to the
ParsedFreeStandingDeclSpec as well.

If there's some more appropriate common point to put this logic, I'm
all ears - but it seems to me there's no common point in Sema for
these two cases so it's either duplicated or could be non-duplicated
if it were up in the Parser (or had a new Sema entry point to call
from there).

Open to ideas - thanks again for the help/review/pointers.

That seems reasonable.

+void ASTContext::addUnnamedTag(const TagDecl *Tag) {
+ if (!Tag->getName().empty() || Tag->getTypedefNameForAnonDecl() ||
+ !isa<CXXRecordDecl>(Tag->getParent()) || Tag->getLinkage() !=
ExternalLinkage)
+ return;

Why do we need to enforce that the parent is a CXXRecordDecl? I think
you can run into the same issue in other situations, no? (Or were you
intentionally putting that off to another patch? I forget.)

+ std::pair<llvm::DenseMap<const DeclContext *, unsigned>::iterator,

P = UnnamedMangleContexts.insert(std::make_pair(Tag->getParent(),

0));

80 columns.

I don't see any other issues.

-Eli

+patch

Comments below.

Hi John, (& cfe-dev)

I mentioned this in person last week & wanted to provide you with some
more details & ask for your opinion.

Backstory:

I originally came across this while trying to use
-Wunused-member-function & found it was flagging code like this:

struct foo {
  struct {
    void func() { ... } // warning that this is 'unused'
  } x;
};

This surprised me, so I looked around & found that this function has
"no linkage". This seemed strange (because I would expect to be able
to call the function from multiple TUs that included the header
defining 'foo', compare its address for equality, & such things) & it
looks like the Right Thing is for func() to have the same linkage as,
say, an inline member function of 'foo' would have.

The standard (3.5[basic.link]) seems to "miss" this case depending on
how you read it:

1) arguably p2, which says that "A name is said to have linkage when
it might denote the same ... function ... as a name introduced by a
declaration in another scope: - When a name has external linkage, the
entity it denotes can be referred to by names from scopes of other
translation units or from other scopes of the same translation unit"

2) p5: "... a member function ... has external linkage if the name of
the class has external linkage" (with an exception only for unnamed
classes (& enumerations) defined in class-scope typedef declarations
such that the class or enumeration has the dypedef name for linkage
purposes (7.1.3)) & there are no rules that seem to govern the linkage
of this unnamed class.
    p8 "Names not covered by these rules have no linkage"

[basic.link]p8: "A type is said to have linkage if and only if [...]
it is an unnamed class or enumeration member of a class with linkage".

I'm going to assume it's an oversight in the standard that the members
of such an unnamed class don't have linkage.

The next step was also to look at the mangling compared to GCC. After
modifying the linkage of functions like this (with this change):

--- lib/AST/Decl.cpp
+++ lib/AST/Decl.cpp
@@ -496,8 +496,7 @@ static LinkageInfo getLVForClassMember(const
NamedDecl *D, bool OnlyTemplate) {
   if (!(isa<CXXMethodDecl>(D) ||
         isa<VarDecl>(D) ||
         isa<FieldDecl>(D) ||
- (isa<TagDecl>(D) &&
- (D->getDeclName() || cast<TagDecl>(D)->getTypedefNameForAnonDecl()))))
+ isa<TagDecl>(D)))
     return LinkageInfo::none();

   LinkageInfo LV;

(& Richard reckons we might be able to simplify that check - just to
eliminate some template cases that still seem to get filtered out by
it)

This looks like a step in the right direction.

I caused one test to fail:
test/CodeGenCXX/template-anonymous-types.cpp. A modified version (to
better investigate GCC 4.7's mangling) looks like this:

struct S {
  enum { FOO = 42 };
  enum { BAR = 42 };
};

struct T {
  enum { FOO = 42 };
  enum { BAR = 42 };
};

template <typename T> struct X {
  T value;
  X(T t) : value(t) {}
  int f() { return value; }
};

template <typename T> int f(T t) {
  X<T> x(t);
  return x.f();
}

void test() {
  (void)f(S::FOO);
  (void)f(S::BAR);
  (void)f(T::FOO);
  (void)f(T::BAR);
}

& with my change we get the right linkage for the instantiations of
'f' (linkonce_odr instead of internal) but the mangling is still
inconsistent with GCC at least. Clang mangles these 4 'f's as:

_Z1fIN1S3$_0EEiT_
_Z1fIN1S3$_1EEiT_
_Z1fIN1T3$_2EEiT_
_Z1fIN1T3$_3EEiT_

GCC 4.7 mangles them as:

_Z1fIN1SUt_EEiT_
_Z1fIN1SUt0_EEiT_
_Z1fIN1TUt_EEiT_
_Z1fIN1TUt0_EEiT_

I don't think we have any class-specific unnamed nested type counter
that would implement that Ut_, Ut0_, ... mangling scheme, though I can
imagine where one might be added (I'm not very familiar with IRGen
though, so I'll certainly be happy to have any pointers about how that
could/should be implemented).

We have a similar scheme already implemented for lambdas; look at
getLambdaManglingNumber() etc.

Thanks for the pointer. Following the lines there I've had a first
blush at this (see attached).

I've not addressed some of the issues Richard brought up but I'd be
happy to take the time to generalize this to handle.

Spent a little time trying to generalize this to handle the function
local case but it was taking a bit too much of my time so I figured
I'd at least get this case handled (which addresses my issue of
-Wunused-function & generally improves the world a little bit at
least).

+ if (!Tag->getName().empty() || Tag->getTypedefNameForAnonDecl() ||
+ !isa<CXXRecordDecl>(Tag->getParent()))
+ return -2;
+
+ std::pair<llvm::DenseMap<const DeclContext *, int>::iterator, bool>
P = UnnamedMangleContexts.insert(std::make_pair(Tag->getParent(),
-1));

getParent() doesn't return a canonical DeclContext.

I'm not entirely sure what bugs this might cause (test cases welcome)
but I believe I've addressed this in the newly attached patch.

It might not matter here, because all decls contained in a RecordDecl
generally have the same parent... this has a bigger impact for
NamespaceDecl, for example, because there are two different Decls
which are semantically the same context.

Thanks for the explanation.

Also, 80 columns.

Right, sorry, I was mostly looking to see if I was generally on the
right track. Fixed, though.

Also, we probably don't want to generate numbers for a decl unless
the mangling is actually externally visible.

Done.

--- include/clang/AST/Decl.h
+++ include/clang/AST/Decl.h
@@ -2486,6 +2486,8 @@ private:
   /// otherwise, it is a null (TypedefNameDecl) pointer.
   llvm::PointerUnion<TypedefNameDecl*, ExtInfo*> TypedefNameDeclOrQualifier;

+ int UnnamedManglingNumber;
+
   bool hasExtInfo() const { return TypedefNameDeclOrQualifier.is<ExtInfo*>(); }
   ExtInfo *getExtInfo() { return TypedefNameDeclOrQualifier.get<ExtInfo*>(); }
   const ExtInfo *getExtInfo() const {

We don't really want to add an extra member to DeclContext just to
handle an obscure edge case.

Used a separate side table in the ASTContext to keep the numbers. (& I
changed the numbers to be simpler - counting from zero instead of -1,
then doing an offset to compute the actual mangle number. Let me know
if you'd prefer it to work the other way)

(now with the actually up-to-date patch)

@@ -2582,8 +2581,9 @@ void
TagDecl::setTypedefNameForAnonDecl(TypedefNameDecl *TDD) {
void TagDecl::startDefinition() {
   IsBeingDefined = true;

- if (isa<CXXRecordDecl>(this)) {
- CXXRecordDecl *D = cast<CXXRecordDecl>(this);
+ getASTContext().addUnnamedTag(this);

Will getTypedefNameForAnonDecl actually return the correct result
here?

Seems not - thanks for the catch.

(I'd like to see a test to make sure that "typedef struct {} x"
doesn't cause us to increment the mangling number, assuming that's
consistent with gcc.)

Test case added & then I had to restructure the code to address this.
Since we don't know which way a type will be mangled until after we've
parsed all the declarators in the group (eg: typedef struct{} *x, *y,
z; - only when we see 'z' do we know that we have a linkage name for
the struct, if we only have x and y, then we don't have such a name &
we need to use the UtX_ naming) I had to inject the logic into
FinalizeDeclaratorGroup (or somewhere near there - perhaps there's a
better spot). This means we miss the original test case failure of:
struct foo { enum { X }; }; since there is no declarator group in tht
instance, just a standalone decl. So I added this to the
ParsedFreeStandingDeclSpec as well.

If there's some more appropriate common point to put this logic, I'm
all ears - but it seems to me there's no common point in Sema for
these two cases so it's either duplicated or could be non-duplicated
if it were up in the Parser (or had a new Sema entry point to call
from there).

Open to ideas - thanks again for the help/review/pointers.

That seems reasonable.

+void ASTContext::addUnnamedTag(const TagDecl *Tag) {
+ if (!Tag->getName().empty() || Tag->getTypedefNameForAnonDecl() ||
+ !isa<CXXRecordDecl>(Tag->getParent()) || Tag->getLinkage() !=
ExternalLinkage)
+ return;

Why do we need to enforce that the parent is a CXXRecordDecl? I think
you can run into the same issue in other situations, no? (Or were you
intentionally putting that off to another patch? I forget.)

I played around with expanding this to the function-local case but had
a few problems. Figured it would be best to separate that out for a
subsequent commit.

+ std::pair<llvm::DenseMap<const DeclContext *, unsigned>::iterator,
> P = UnnamedMangleContexts.insert(std::make_pair(Tag->getParent(),
0));

80 columns.

Hrm, sorry - not sure how that snuck back in. Thanks for the catch.

I don't see any other issues.

Thanks for the help/review. Committed in r167906.

- David