Improvement: diagnose undefined behavior: within a translation unit the same identifier appears with both internal and external linkage

[pmor13]

static int x;
void f(void)
{
    int x;
    {
        extern int x;
    }
}

Expected diagnostics:

t0.c:7:20: error: the same identifier 'x' appears with both internal and external linkage
    7 |         extern int x;

Actual diagnostics:

<nothing>

C11:

If, within a translation unit, the same identifier appears with both internal and external linkage, the behavior is undefined.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

[AaronBallman]

Agreed that we should issue a diagnostic in this case.

C2x 6.2.2p7: If, within a translation unit, the same identifier appears with both internal and external linkage, the behavior is undefined.
C++2b [dcl.stc]p6: All declarations for a given entity shall give its name the same linkage.

(There's a lot of good examples for test cases in C++ in http://eel.is/c++draft/dcl.stc#example-1)

[pmor13]

@jakevossen5 @junaire Any interest / time / motivation in fixing this issue?

[junaire]

I can have a try... But I'm not super familiar with this code path. Would you like to explain it a little bit to me? Thanks! @AaronBallman

[AaronBallman]

I can have a try... But I'm not super familiar with this code path. Would you like to explain it a little bit to me? Thanks! @AaronBallman

I think the most logical place I'd start looking is in Sema::MergeVarDecl() because that's the place where we know we have a new declaration and we saw a previous declaration of the same name that's also in scope.

[pmor13]

@junaire I see it this way: within a translation unit:

1. Obtain a list of all identifiers with internal or external linkages: e.g. x:I, y:E, z:I, x:E, z:I
2. For the same identifiers: check that linkages match: either external, either internal.
3. If unmatched cases found, then report as "error: the same identifier 'x' appears with both internal and external linkage (the behavior is undefined)".

[elhewaty]

Hello, Is there any one working on this?
I am new to LLVM, I Wish I could help with this.

[EugeneZelenko]

@elhewaty: Usual signs of work are filled Assignees field or Differential patch mentioned in issue.

[junaire]

Hello, Is there any one working on this?

I'm sorry lately I've been busy with other stuff and don't look into the issue too much, feel free to take over!

I am new to LLVM, I Wish I could help with this.

Welcome! don't hesitate to ask questions if you have any problems!

[elhewaty]

@junaire
In Sema::MergeVarDecl I found the following condition:

// Variables with external linkage are analyzed in FinalizeDeclaratorGroup.

  // FIXME: The test for external storage here seems wrong? We still
  // need to check for mismatches.
  if (!New->hasExternalStorage() && !New->isFileVarDecl() &&
      // Don't complain about out-of-line definitions of static members.
      !(Old->getLexicalDeclContext()->isRecord() &&
        !New->getLexicalDeclContext()->isRecord())) {
    Diag(New->getLocation(), diag::err_redefinition) << New->getDeclName();
    Diag(OldLocation, PrevDiag);
    return New->setInvalidDecl();
  }

Is this a good point to start from?
If I want to store the variables, How about adding a new class member of the type
map<Identifier, unordered_set<linkage>> and check before adding any ID, check its linkage.

[pmor13]

@junaire Just in case you've missed notification. See message above from elhewaty dated October 6th.

[AaronBallman]

Sorry @elhewaty, I didn't realize you hadn't gotten an answer to your question! Are you still interested in working on this (it's totally fine if you're not)?

Is this a good point to start from?

Yeah, that's roughly where I would start to look.

If I want to store the variables, How about adding a new class member of the type map<Identifier, unordered_set<linkage>> and check before adding any ID, check its linkage.

I don't think you should have to store anything in a map like that. By the point we're calling MergeVarDecl, we've already determined "this variable and that variable are quite possibly referring to the same thing" and we're checking if that's the case. So you should only have to compare between New and Old because additional declarations will get additional calls to MergeVarDecl. These AST nodes already track their own linkage and identifier, so I think you've got everything you should need.

[elhewaty]

Hello @AaronBallman, I didn't see your reply sorry for that. I started contributing to LLVM middle end lately by working on missing optimization/ instcombine issues.
I will work on this today, I am interested in LLVM/Clang, and I wish I could be a maintainer soon,
I don't know how, but I will continue working.
Thanks for your help.