Page MenuHomePhabricator

PR5172: Fix for a bug in pragma redefine_extname implementation.
ClosedPublic

Authored by andreybokhanko on Jun 2 2015, 8:31 AM.

Details

Summary

PR5172: Fix for a bug in pragma redefine_extname implementation: it doesn't work correctly when a structure is declared before pragma and then a function with the same name declared after pragma.

Diff Detail

Repository
rL LLVM

Event Timeline

andreybokhanko retitled this revision from to PR5712: Fix for a bug in pragma redefine_extname implementation..
andreybokhanko updated this object.
andreybokhanko edited the test plan for this revision. (Show Details)
andreybokhanko added reviewers: theraven, rsmith.
andreybokhanko retitled this revision from PR5712: Fix for a bug in pragma redefine_extname implementation. to PR5172: Fix for a bug in pragma redefine_extname implementation..
andreybokhanko updated this object.
andreybokhanko added a subscriber: Unknown Object (MLST).
andreybokhanko added a reviewer: aaron.ballman.

Aaron, thank you for the review. I did as you suggested; patch updated.

Patch updated again (the only difference is that a pair of curly braces went away).

David [Chisnall], Aaron wrote (http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20150601/130212.html) that:

"Changes look good... However, since I don't know about the semantics of this particular pragma, I'll let someone else give the final LGTM."

You are the original author of this pragma support; could you, please, LGTM my fix?

Andrey

theraven edited edge metadata.Jun 4 2015, 5:15 AM

I'm also not completely sure of the semantics - I just made it work well enough for the Solaris headers that libc++ needed to work. The changes look fine to me though.

Have they been tested with the various things in compiler-rt that use this pragma? We currently use it as a hack because clang doesn't allow you to implement functions that have the same name as builtins (even when those builtins are ones that need implementing by a function of the same name on some archs).

I'm also not completely sure of the semantics - I just made it work well enough for the Solaris headers that libc++ needed to work. The changes look fine to me though.

The semantic is described here: http://docs.oracle.com/cd/E19205-01/819-5265/bjacu/index.html

IMHO, clang implements it right; the only thing I did is fixed a corner case.

Have they been tested with the various things in compiler-rt that use this pragma? We currently use it as a hack because clang doesn't allow you to implement functions that have the same name as builtins (even when those builtins are ones that need implementing by a function of the same name on some archs).

I did "make check-all" + ran a few internal Intel tests.

What is the best way to test compiler-rt with my changes?

Andrey

David, I checked out compiler-rt and ran "make check-all". No new fails (several AddressSanitizer tests fail, but they do so both with and without my changes).

Is this the testing you wanted?

Andrey

Should be fine then, though I'm not sure if the code for the C11 atomic helpers is compiled by default (it needs to go in a shared lib to work).

Should be fine then, though I'm not sure if the code for the C11 atomic helpers is compiled by default (it needs to go in a shared lib to work).

OK, thanks!

Is it OK to commit this or should I wait for explicit "LGTM" from someone?

Andrey

rsmith edited edge metadata.Jun 5 2015, 12:22 PM

I think the right way to handle this would be to filter out declarations that are neither FunctionDecls nor VarDecls after performing the lookup. (According to the specification, we should also filter out declarations with internal linkage.) We should also filter out internal-linkage declarations and non-TU-scope declarations when we perform the deferred handling of ExtnameUndeclaredIdentifiers in ActOnVariableDeclarator and ActOnFunctionDeclarator.

That said, I'm not sure why your testcase was failing -- LookupSingleName in LookupOrdinaryName mode in C should not find tag names, so we should have deferred adding the attribute already. Is a typedef missing from the testcase, perhaps?

andreybokhanko edited edge metadata.

Patch updated in response to Richard Smith's comments.

I think the right way to handle this would be to filter out declarations that are neither FunctionDecls nor VarDecls after performing the lookup. (According to the specification, we should also filter out declarations with internal linkage.)

OK -- I did as you suggested. Please review updated patch.

We should also filter out internal-linkage declarations and non-TU-scope declarations when we perform the deferred handling of ExtnameUndeclaredIdentifiers in ActOnVariableDeclarator and ActOnFunctionDeclarator.

This probably belongs to a separate patch (for starters, I don't have a test demonstrating that compiler is currently acting incorrectly).

That said, I'm not sure why your testcase was failing -- LookupSingleName in LookupOrdinaryName mode in C should not find tag names, so we should have deferred adding the attribute already. Is a typedef missing from the testcase, perhaps?

You mean modifying it like this:

typedef struct {

int f;

} statvfs64;

?

Then compiler issue an error: "redefine_extname.c:10:5: error: redefinition of 'statvfs64' as different kind of symbol". No errors are issued in the original test.

rsmith accepted this revision.Jun 9 2015, 11:23 AM
rsmith edited edge metadata.

Ah, the new test makes perfect sense. Thanks!

test/CodeGen/redefine_extname.cpp
1 ↗(On Diff #27378)

This test should be in test/CodeGenCXX now.

This revision is now accepted and ready to land.Jun 9 2015, 11:23 AM
andreybokhanko added a reviewer: amusman.

Richard, thank you for the review!

I moved redefine_extname.cpp to test/CodeGenCXX/.

I don't have commit access; Alexander [Musman], could you, please, commit this patch?

This revision was automatically updated to reflect the committed changes.