LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 16014 - -fms-compatibility should diagnose failed lookups in the presence of a dependent base
Summary: -fms-compatibility should diagnose failed lookups in the presence of a depend...
Status: RESOLVED FIXED
Alias: None
Product: clang
Classification: Unclassified
Component: Frontend (show other bugs)
Version: unspecified
Hardware: PC Windows NT
: P normal
Assignee: Unassigned Clang Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-15 10:24 PDT by Reid Kleckner
Modified: 2013-10-21 11:08 PDT (History)
3 users (show)

See Also:
Fixed By Commit(s):


Attachments
minimal dependent base lookup failure test case (226 bytes, text/plain)
2013-05-15 10:24 PDT, Reid Kleckner
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Reid Kleckner 2013-05-15 10:24:19 PDT
Created attachment 10513 [details]
minimal dependent base lookup failure test case

This is a variation on the old "unqualified lookup in dependent base" problem.  Normally clang errors out on code like the following, but in -fms-compatibility mode, it fails to instantiate the method and actually makes it to codegen without crashing or erroring out.  Eventually I get a link error because nobody else creates this instantiation.

$ cat min_etw.cc
template <int N> struct DepBase {
  int header; // Commenting out gives the same result with -fms-compatibility.
};
template <int N> struct Depends : public DepBase<N> {
  int *get() { return &header; }  // header does not resolve
};
int *foo() {
  Depends<5> event;
  return event.get();
}

$ clang -cc1 -fms-extensions -triple i686-pc-linux min_etw.cc -emit-llvm -o t.ll
min_etw.cc:4:25: error: use of undeclared identifier 'header'
  void *get() { return &header; }  // header does not resolve
                        ^
1 error generated.
# This is correct, it's part of the FAQ:
# http://clang.llvm.org/compatibility.html#dep_lookup_bases

$ clang -cc1 -fms-compatibility -triple i686-pc-linux min_etw.cc -emit-llvm -o t.ll
# Exit code 0 and no diagnostic.

$ grep '_Z.*get' t.ll
  %call = call i32* @_ZN7DependsILi5EE3getEv(%struct.Depends* %event)
declare i32* @_ZN7DependsILi5EE3getEv(%struct.Depends*) #1
# get is declared, not defined.
Comment 1 Richard Smith 2013-05-15 11:41:40 PDT
I would have expected msvc to accept that code (with 'header' uncommented). Does it not?
Comment 2 Reid Kleckner 2013-05-15 16:15:32 PDT
Yes, it accepts it.  There's two bugs:

1. If delayed name lookup fails, there should be a diagnostic, not a silent failure to instantiate leading to link errors.

2. If delayed name lookup succeeds, then we should instantiate the method.

I haven't looked, but my guess is that the delayed dependent base lookup is unimplemented in some way.

For my immediate problem, I just fixed the code, but a diagnostic would be good.
Comment 3 Reid Kleckner 2013-10-10 19:15:13 PDT
I hit this again and debugged it all over from scratch.  Clearly I should've fixed it the first time.  :)

This bug only occurs when you take the address of a member of a dependent base.  This test case fails to link because we fail to instantiate get2 and we fail to emit a diagnostic.

struct A { int a; };
template <typename T> struct B : T {
  int get() { return a; }
  int *get2() { return &a; }
};
int main() {
  B<A> b;
  b.get();
  b.get2();
}

The AST shows shows that the instantiation of get2 is an invalid decl, while get() is fine.

There are two codepaths in ActOnDependentIdExpression, one for addressof and one for not, and perhaps in this instance we should be taking the first code path.
Comment 4 Reid Kleckner 2013-10-14 13:52:20 PDT
Richard came up with this testcase offline:

  template <typename T>
  struct Foo : T {
    struct Bar {
      int baz() { return a; } // MSVC finds A::a
    };
  };
  struct A { static const int a = 1; };
  int main() {
    Foo<A>::Bar b;
    return b.baz();
  }
  int a = 0;  // clang finds ::a.

For full compatibility, we'd need to add a new AST node for all unqualified ids in templates that delays lookup until instantiation time, essentially removing the last vestiges of two-phase name lookup from -fms-compatibility.  Note that this node could also be useful during error recovery if lookup fails in a class template with dependent bases.

The question is, do we want to go this far?

Personally, I'd like to warn on this extension.  If we assume most of our users make their code -Wmicrosoft clean, then it doesn't make much sense to maintain this highly divergent code path for template parsing.  So I'd like to try to get by with the current solution of turning failed lookups into CXXDependentScopeMemberExprs.  Does that seem reasonable?
Comment 5 Reid Kleckner 2013-10-21 11:08:37 PDT
Fixed by r192860 by assuming unknown unqualified ids are dependent member exprs and without doing fully delayed lookup.