__func__ should be of type char[], not char*

See http://llvm.org/bugs/show_bug.cgi?id=1903. Basic test:

int abcdefghi12(void) {
  static const char* s = __func__;
  const char (*ss)[12] = &__func__;
  return sizeof(__func__);
}

should not give any errors or warnings, and should return 12.

The change in Sema/SemaExpr.cpp is changing the type of __func__ appropriately.

The changes in AST/Expr.cpp makes __func__ be recognized as an lvalue
with static storage duration.

The change in CodeGen/CGExpr.cpp keeps codegen working in the presence
of the new type for __func__.

-Eli

fixpredef.txt (1.82 KB)

Very nice, applied:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080107/003745.html

I wrapped a few lines to 80 columns and changed strlen to ask the identifier its length directly.

Also note that your test above works better, but it is still not entirely happy:

predef.c:2:25: error: initializer element is not constant
  static const char* s = __func__;
                         ^~~~~~~~

Thanks Eli,

-Chris

Very nice, applied:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080107/003745.html

Cool.

I have a minor correction to the patch: __func__ is a static const
char[], so the type in the array is actually
"Context.CharTy.getQualifiedType(QualType::Const)". Test:
void a() {
__func__[0] = 'a';
}
should give an error with clang -fsyntax-only.

Another small thing I didn't think of when I submitted the patch:
CurFunctionDecl needs to be null-checked. Testcase:
char* a = __func__; // File scope declaration

gcc's exact behavior here is kind of weird, though... it considers
this mistake to be only a warning, and it lets through __FUNCTION__
without even complaining; both __func__ and __FUNCTION__ end up
resolving to an empty string. I'm not sure if clang needs to copy gcc
here.

I wrapped a few lines to 80 columns and changed strlen to ask the
identifier its length directly.

Also note that your test above works better, but it is still not
entirely happy:

predef.c:2:25: error: initializer element is not constant
  static const char* s = __func__;
                         ^~~~~~~~

I think that's dependent on my patch for compound literals... clang
doesn't currently deal with constant-testing of ImplicitCastExpr
correctly.

-Eli

Very nice, applied:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080107/003745.html

Cool.

I have a minor correction to the patch: __func__ is a static const
char[], so the type in the array is actually
"Context.CharTy.getQualifiedType(QualType::Const)". Test:
void a() {
__func__[0] = 'a';
}
should give an error with clang -fsyntax-only.

Another small thing I didn't think of when I submitted the patch:
CurFunctionDecl needs to be null-checked. Testcase:
char* a = __func__; // File scope declaration

Both fixed, thanks!
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080107/003746.html

Steve/Fariborz, is __func__ valid in an objc method?

gcc's exact behavior here is kind of weird, though... it considers
this mistake to be only a warning, and it lets through __FUNCTION__
without even complaining; both __func__ and __FUNCTION__ end up
resolving to an empty string. I'm not sure if clang needs to copy gcc
here.

I agree that that is pretty silly.

Also note that your test above works better, but it is still not
entirely happy:

I think that's dependent on my patch for compound literals... clang
doesn't currently deal with constant-testing of ImplicitCastExpr
correctly.

Ok, thanks again Eli!

-Chris

Very nice, applied:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080107/003745.html

Cool.

I have a minor correction to the patch: __func__ is a static const
char[], so the type in the array is actually
"Context.CharTy.getQualifiedType(QualType::Const)". Test:
void a() {
__func__[0] = 'a';
}
should give an error with clang -fsyntax-only.

Another small thing I didn't think of when I submitted the patch:
CurFunctionDecl needs to be null-checked. Testcase:
char* a = __func__; // File scope declaration

Both fixed, thanks!
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080107/003746.html

Steve/Fariborz, is __func__ valid in an objc method?

__func__ refers to synthesized method name. For example in:

@interface I
- (int) Meth;
@end

__func__ will refer to:

"-[I Meth]"

- fariborz

k

Steve/Fariborz, is __func__ valid in an objc method?

__func__ refers to synthesized method name. For example in:

@interface I
- (int) Meth;
@end

__func__ will refer to:

"-[I Meth]"

Ok, is __FUNCTION__ and __PRETTY_FUNCTION__ also allowed? If so, this is close to the right patch I think:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080107/003747.html

The problem is that it only reserves space for "foo:bar:". If you know how to get the right length, please fix :slight_smile:

-Chris

Steve/Fariborz, is __func__ valid in an objc method?

__func__ refers to synthesized method name. For example in:

@interface I
- (int) Meth;
@end

__func__ will refer to:

"-[I Meth]"

Ok, is __FUNCTION__ and __PRETTY_FUNCTION__ also allowed?

Yes. In gcc they are replaced with the synthesize method name string as well.

If so, this is close to the right patch I think:

Yes. As you pointed out, 'length' of array type is the problem. This is not a feature that I have seen used in objective-c. But nevertheless....
Either length of array should be ASTized or the entire macro should be represented by an 'AST'. This of course has consequences in type matching, sizeof, etc. down the road. I will look at this later on.

- Fariborz

If so, this is close to the right patch I think:

Yes. As you pointed out, 'length' of array type is the problem. This is not a feature that I have seen used in objective-c. But nevertheless....
Either length of array should be ASTized or the entire macro should be represented by an 'AST'. This of course has consequences in type matching, sizeof, etc. down the road. I will look at this later on.

It's not a macro, it's a 'predefined identifier' :). It already is represented explicitly in the AST, but we need to know it's type for the AST node. Thanks Fariborz,

-Chris

If so, this is close to the right patch I think:

Yes. As you pointed out, 'length' of array type is the problem. This is not a feature that I have seen used in objective-c. But nevertheless....
Either length of array should be ASTized or the entire macro should be represented by an 'AST'. This of course has consequences in type matching, sizeof, etc. down the road. I will look at this later on.

It's not a macro, it's a 'predefined identifier' :). It already is represented explicitly in the AST, but we need to know it's type for the AST node. Thanks

If it is an AST already, this is great. We need to have an array type of TBD length. So, can fool type comparison of otherwise compatible arrays. But let's talk about this later.

- Fariborz