This is an archive of the discontinued LLVM Phabricator instance.

[Modules] Implement ODR-like semantics for tag types in C/ObjC
ClosedPublic

Authored by bruno on Apr 6 2017, 11:16 AM.

Details

Summary

Allow ODR for ObjC/C in the sense that we won't keep more that one definition around (merge them). However, ensure the decl pass the structural compatibility check in C11 6.2.7/1, for that, reuse the structural equivalence checks used by the ASTImporter.

Few other considerations:

  • Create error diagnostics for tag types mismatches and thread them into the structural equivalence checks.
  • Note that by doing this we only support redefinition between types that are considered "compatible types" by C11.

This is mixed approach of the suggestions discussed in http://lists.llvm.org/pipermail/cfe-dev/2017-March/053257.html

Depends on D31777

Diff Detail

Repository
rL LLVM

Event Timeline

bruno created this revision.Apr 6 2017, 11:16 AM

Thanks!
Added a couple of superficial coding style comments.

I think there should also be a test for C99 and one for C11 (and potentially ObjC2).

include/clang/AST/ASTStructuralEquivalence.h
52 ↗(On Diff #94400)

No need to use \brief in new code any more. We are compiling with the Doxygen autobrief option.

lib/Parse/ParseDecl.cpp
4316 ↗(On Diff #94400)

I think the LLVM coding style wants this to be

{
    // Bail out.
    DS.SetTypeSpecError();
    return;
}

or

// Bail out.
return DS.SetTypeSpecError();
lib/Parse/ParseDeclCXX.cpp
1913 ↗(On Diff #94400)

.

lib/Sema/SemaDecl.cpp
13386 ↗(On Diff #94400)

Does ObjC2 imply ObjC1?

lib/Sema/SemaType.cpp
7085 ↗(On Diff #94400)

Comment should go on the declaration.

bruno added inline comments.Apr 6 2017, 6:11 PM
include/clang/AST/ASTStructuralEquivalence.h
52 ↗(On Diff #94400)

Sure, updated D31777 to also remove the \briefs in the transition

lib/Parse/ParseDecl.cpp
4316 ↗(On Diff #94400)

OK

lib/Sema/SemaDecl.cpp
13386 ↗(On Diff #94400)

Yep!

bruno updated this revision to Diff 94471.Apr 6 2017, 6:12 PM

Update patch on top of Adrian comments

rsmith added inline comments.Apr 19 2017, 4:55 PM
include/clang/Sema/Sema.h
1464 ↗(On Diff #94471)

Typo 'abd'

lib/Parse/ParseDecl.cpp
4236–4237 ↗(On Diff #94471)

Do we really need both of these? The new stuff seems to be a natural extension of SkipBodyInfo, to say "parse it, check it's the same as the previous definition, then throw the new one away".

lib/Sema/SemaDecl.cpp
12929 ↗(On Diff #94471)

No void here, please.

12962 ↗(On Diff #94471)

"act on tag decl" -> ActOnTag?

13383–13384 ↗(On Diff #94471)

Plurality mismatch in "decl pass"

lib/Sema/SemaExpr.cpp
2198–2209 ↗(On Diff #94471)

This is gross. In order to make this work, you'd need to propagate the IgnorePrevDef flag through the entire expression parsing machinery.

Instead of this, how about deferring making the old definition visible until after you complete parsing the new one and do the structural comparison?

bruno marked 6 inline comments as done.Apr 20 2017, 7:11 PM
bruno added inline comments.
lib/Parse/ParseDecl.cpp
4236–4237 ↗(On Diff #94471)

No specific reason. I'll augment SkipBodyInfo with two new fields then (a) a flag for the check and (b) the new tag.

lib/Sema/SemaExpr.cpp
2198–2209 ↗(On Diff #94471)

Thanks for pointing it out, I totally missed this approach. Your suggestion works and I'll change the patch accordingly. However, IgnorePrevDef still needs to be threaded in ParseEnumBody and ActOnEnumConstant in order to prevent the latter to emit err_redefinition_of_enumerator-like diagnostics.

bruno updated this revision to Diff 96080.Apr 20 2017, 7:13 PM
bruno marked 2 inline comments as done.

Update the patch after Richard's suggestions

rsmith added inline comments.May 9 2017, 12:19 PM
include/clang/Sema/Sema.h
1464 ↗(On Diff #94471)

Typo 'compatibale' =)

lib/Parse/ParseDecl.cpp
4307 ↗(On Diff #96080)

Commented-out code, please remove.

4313–4321 ↗(On Diff #96080)

The parser shouldn't be doing this itself, please move this to a function on Sema (ActOnDuplicateDefinition or something) and call that from here.

lib/Sema/SemaExpr.cpp
2198–2209 ↗(On Diff #94471)

I think the problem is that we don't take module visibility into account when doing redefinition checking for enumerators. Instead of passing through this flag, we should probably just ignore hidden declarations when checking for a redefinition of an enumerator.

bruno marked 3 inline comments as done.May 19 2017, 9:56 AM

I think the problem is that we don't take module visibility into account when doing redefinition checking for enumerators. Instead of passing through this flag, we should probably just ignore hidden declarations when checking for a redefinition of an enumerator.

Right, now that the patch changed to only merge the definition after the structural check it does makes sense to do that.

bruno updated this revision to Diff 99585.May 19 2017, 9:58 AM

Update the patch to address @rsmith comments and rebase

rsmith added inline comments.May 19 2017, 11:48 AM
include/clang/Parse/Parser.h
1956–1957 ↗(On Diff #99585)

Please remove this (now unused) parameter.

lib/Parse/ParseDecl.cpp
4391 ↗(On Diff #99585)

Please revert this no-op change (left over from when you were passing in IgnorePrevDef).

lib/Sema/SemaDecl.cpp
15423 ↗(On Diff #99585)

The isFromASTFile check here is not correct: whether a declaration is from an AST file is independent of whether it might be not visible due to being in a module (local submodule visibility rules allow a local declaration to not be visible). The Modules check is also not correct: we do visibility checking for local #includes of module headers under -fmodules-local-submodule-visibility -fno-modules.

We also shouldn't diagnose ambiguity if we find two different hidden previous declarations, but we will do so (within LookupSingleName) with this approach.

Digging into this a bit more, I think the root of the problem is that the ND->isExternallyVisible() call in LookupResult::isHiddenDeclarationVisible is wrong. Redeclaration lookup should never find hidden enumerators in C, because they do not have linkage (C11 6.2.2/6). (The same is true in C++, but I don't know whether we can apply the same thing there too, due to the different merging rules.) A function foo in some source file should not conflict with an enumerator foo in a non-imported module, for instance.

rsmith edited edge metadata.Jun 26 2017, 1:38 PM

What's the status of this patch? https://reviews.llvm.org/D34510 is touching on a related issue, and we need to resolve how we're going to deal with ODR-like issues in C to determine how to address the issue that it is tackling.

bruno updated this revision to Diff 104881.Jun 30 2017, 9:54 AM

Digging into this a bit more, I think the root of the problem is that the ND->isExternallyVisible() call in LookupResult::isHiddenDeclarationVisible is wrong. Redeclaration lookup should never find hidden enumerators in C, because they do not have linkage (C11 6.2.2/6). (The same is true in C++, but I don't know whether we can apply the same thing there too, due to the different

merging rules.) A function foo in some source file should not conflict with an enumerator foo in a non-imported module, for instance.

The linkage of an enumerator should probably be VisibleNoLinkage, and isHiddenDeclarationVisible should probably be checking hasExternalFormalLinkage...

Implemented this suggestions from last round of review. Change didn't seem to have side effects (at least from clang tests). @rsmith, what do you think? (I should probably split this small change into a different commit when the patch is accepted)

rsmith accepted this revision.Jun 30 2017, 2:09 PM
rsmith added inline comments.
lib/Sema/SemaDecl.cpp
13693–13694 ↗(On Diff #104881)

I don't think this should require C11 -- in C89 the corresponding rule is in 6.1.2.6/1, so the rule exists in all languages that Clang supports.

This revision is now accepted and ready to land.Jun 30 2017, 2:09 PM
This revision was automatically updated to reflect the committed changes.