1902891 - duplicate structured records with different canonical field type representation

Status: RESOLVED FIXED

(Webtools :: Searchfox, enhancement)

Description

[Tooru Fujisawa [:arai]]

https://searchfox.org/mozilla-central/query/default?q=field-layout%3A%27js%3A%3Agc%3A%3AGCRuntime%27

This shows that there are 2 types for systemZone field:

• class js::ProtectedDataNoCheckArgs<class js::CheckMainThread<js::AllowedHelperThread::None>, class JS::Zone *>
• class js::ProtectedDataNoCheckArgs<class js::CheckMainThread<enum AllowedHelperThread::None>, class JS::Zone *>

The difference is the AllowedHelperThread::None part, whether it has js:: prefix or enum prefix.

When I look into the GCRuntime.h analysis file before merge, there are 2 structured records for all platform except for android-armv7,
where the first one has enum prefix and the second one has js:: prefix.

Comment 1

[Tooru Fujisawa [:arai]]

when compiling the code with mozsearch-plugin, in most case, the class GCRuntime declaration generates field type with js:: prefix for AllowedHelperThread::None, but if the compilation unit contains js/src/threading/ProtectedData.cpp, especially the following template instantiation, the class GCRuntime generates field type with enum prefix.

https://searchfox.org/mozilla-central/rev/5695e19e553e8087a94d1aab945e82771ea825ee/js/src/threading/ProtectedData.cpp#66

template class CheckMainThread<AllowedHelperThread::None>;

So I guess this is an issue around how clang generate the canonical type string representation, depending on whether the instantiation exists in the compilation unit or not.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Thanks for digging into the cause of the over-proliferation of structured representation variants[1]! cc'ing :botond in case he might have some thoughts/awareness on this.

1: Restating: Our clang plugin de-duplicates records based on string equivalence, so if the structured info emission does not always produce the exact same output, the merger logic which uses the hash of the string representation of the structured JSON records to figure out our variants will end up with a bunch of extra variants. (In general I was assuming this could not happen during initial implementation for simplicity, but I think this was obviously false by the time the structured representation stuff landed from my fancy-branch field-layout prototype. Just it was less bad then.)

Comment 3

[Tooru Fujisawa [:arai]]

Looks like we can control it by PrintingPolicy parameter of QualType::getAsString.

https://searchfox.org/llvm/rev/47f8b85b3d81ed3578cb3b8f80d69ce307e0c883/clang/lib/AST/TypePrinter.cpp#2537-2545

std::string QualType::getAsString() const {
  return getAsString(split(), LangOptions());
}

std::string QualType::getAsString(const PrintingPolicy &Policy) const {
  std::string S;
  getAsStringInternal(S, Policy);
  return S;
}

the policy is eventually consumed by the following:

https://searchfox.org/llvm/rev/47f8b85b3d81ed3578cb3b8f80d69ce307e0c883/clang/lib/AST/TypePrinter.cpp#1493-1504

if (auto *S = dyn_cast<ClassTemplateSpecializationDecl>(D)) {
  const TemplateParameterList *TParams =
      S->getSpecializedTemplate()->getTemplateParameters();
  const ASTTemplateArgumentListInfo *TArgAsWritten =
      S->getTemplateArgsAsWritten();
  IncludeStrongLifetimeRAII Strong(Policy);
  if (TArgAsWritten && !Policy.PrintCanonicalTypes)
    printTemplateArgumentList(OS, TArgAsWritten->arguments(), Policy,
                              TParams);
  else
    printTemplateArgumentList(OS, S->getTemplateArgs().asArray(), Policy,
                              TParams);

and I do see the difference in the following, when the compilation unit contains the template instantiation.

CanonicalFieldType.getAsString(); // generates with `enum ` prefix

PrintingPolicy Policy((LangOptions()));
Policy.PrintCanonicalTypes = true;
CanonicalFieldType.getAsString(Policy); // generates with `js::`

I'll apply the workaround for all callsites the field type (apparently other cases are not related to merge).

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Bug 1902891 - Do not use written template args for canonical field type. r?asuth!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f81b97d6004a
Do not use written template args for canonical field type. r=asuth

Comment 6

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/f81b97d6004a