This is an archive of the discontinued LLVM Phabricator instance.

-fmodules-codegen should not emit extern templates
ClosedPublic

Authored by llunak on Nov 3 2019, 1:38 PM.

Details

Summary

See the test for a testcase. If a header contains 'extern template', then the template should be provided somewhere by an explicit instantiation, so it is not necessary to generate a copy. Worse (at least with PCH -building-pch-with-obj), the current code actually leads to an unresolved symbol, because the PCH's object file will not actually contain functions from such a template (because of the GVA_AvailableExternally?), but the object file for the explicit instantiation will not contain them either because it will be blocked by the information provided by the PCH. I don't know if the same problem exists with modules (nor I know how to check for sure), all tests pass. If the problem is not there for modules, I can make the change PCH-specific.

Diff Detail

Event Timeline

llunak created this revision.Nov 3 2019, 1:38 PM

Hey - thanks for this! Does look like it reproduces in modules:

foo.h:

#pragma once

template <typename T>
struct outer {
static void func() {
}
};

template<typename T>
void func() {
}

extern template struct outer<int>;
extern template void func<int>();

inline void caller() {
  outer<int>::func();
  func<int>();
}

foo.modulemap:

module foo { header "foo.h" }

use.cpp:

#include "foo.h"
template struct outer<int>;
template void func<int>();
int main() {
}

Commands:

clang-tot -cc1 -fmodules-codegen -fmodules -emit-module -fmodule-name=foo foo.modulemap  -x c++ -o foo.pcm
clang-tot -cc1 -fmodules -fmodule-file=foo.pcm use.cpp -emit-obj
clang-tot -c foo.pcm
clang-tot foo.o use.o

This test case exercises both a member function of a class template, and a standalone function template. I'd like to better understand why this shows up for function templates but not member functions of class templates - perhaps there's somewhere the solution to both cases can be unified.

Ah, if I mark the standalone function template 'inline' (the implicit linkage of member functions) then I get the same failure for both. Haven't tested whether the fix is the same fix for both yet.

Ah, if I mark the standalone function template 'inline' (the implicit linkage of member functions) then I get the same failure for both. Haven't tested whether the fix is the same fix for both yet.

With my patch there are no errors with your testcase, even with the 'inline' added.

Do you need some more information about the patch? It'd be nice if this could make it into 10.0.

dblaikie added inline comments.Jan 13 2020, 2:55 PM
clang/test/PCH/codegen-extern-template.cpp
1–9 ↗(On Diff #227635)

Please reduce the test to not rely on linking - it should check for the IR output of clang (not object output) for the symbols that should/shouldn't be present.

Specifically the main function is probably unneeded - and only the explicit template specialization definition is needed. The test would then verify that the required function definition is present in the IR output.

Also - to simplify this, could you change this to use an inline function template - rather than a member function? It'd make it more clear that it's the function template with 'inline' (rather than relying on the implicit inline from member functions).

clang/test/PCH/codegen-extern-template.h
1–16 ↗(On Diff #227635)

Please run this (& the .cpp file) through clang-format, and change the #pragma once to standard header include guards (#define/etc).

llunak updated this revision to Diff 238086.Jan 14 2020, 1:24 PM

I've updated the test as requested.

However I've noticed that a PCH-based test for this relies on https://reviews.llvm.org/D69585 . The fix works (of course), but it requires a PCH/module with the instantiation.

llunak updated this revision to Diff 238089.Jan 14 2020, 1:34 PM

This version uses a module based on the code posted above.

dblaikie accepted this revision.Jan 14 2020, 1:37 PM

Looks good - thanks for your patience!

This revision is now accepted and ready to land.Jan 14 2020, 1:37 PM
This revision was automatically updated to reflect the committed changes.