compnerd added inline comments.
================ Comment at: clang/include/clang/Driver/Options.td:626 HelpText<"Use the LLVM representation for assembler and object files">; +def emit_ifso : Flag<["-"], "emit-interface-stubs">, Flags<[CC1Option]>, Group<Action_Group>, + HelpText<"Generate Inteface Stub Files.">; ---------------- I thought that we were going to add `experimental` to this for the time being? ================ Comment at: clang/include/clang/Driver/Options.td:628 + HelpText<"Generate Inteface Stub Files.">; +def ifso_version_EQ : Joined<["-"], "interface-stubs-version=">, Flags<[CC1Option]>; def exported__symbols__list : Separate<["-"], "exported_symbols_list">; ---------------- Personally, I have a slight preference for the separate version rather than the joined. ================ Comment at: clang/include/clang/Driver/Types.def:91 TYPE("ast", AST, INVALID, "ast", "u") +TYPE("ifs", IFS, INVALID, "ifs", "u") TYPE("pcm", ModuleFile, INVALID, "pcm", "u") ---------------- What about `ifo` instead of `ifs`? ================ Comment at: clang/include/clang/Frontend/FrontendActions.h:128 +}; +// Support different ifso formats this way: +class GenerateIfsoObjYamlExpV1Action : public GenerateIFSOAction { ---------------- Not sure if the comment adds anything. But a newline before these would be nice. ================ Comment at: clang/include/clang/Frontend/FrontendActions.h:134 +}; +class GenerateIfsoTbeExpV1Action : public GenerateIFSOAction { +protected: ---------------- A newline between the classes would be nice. ================ Comment at: clang/include/clang/Frontend/FrontendOptions.h:91 + /// Generate Interface Stub Files. + GenerateIfsoObjYamlExpV1, + GenerateIfsoTbeExpV1, ---------------- As per LLVM style, initialisms are not downcased. This should be `GenerateIfSoObjYAMLExpV1`. I think I prefer `GenerateInterfaceYAMLExpV1` ================ Comment at: clang/include/clang/Frontend/FrontendOptions.h:92 + GenerateIfsoObjYamlExpV1, + GenerateIfsoTbeExpV1, + ---------------- Similar. I think I prefer `GenerateInterfaceTBEExpV1` ================ Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3590 + Twine("-interface-stubs-version=") + + Args.getLastArgValue(options::OPT_ifso_version_EQ))); + } ---------------- Please ensure that the value being passed is valid or emit a diagnostic. ================ Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1649 + case OPT_emit_ifso: + Opts.ProgramAction = frontend::GenerateIfsoTbeExpV1; + if (Args.hasArg(OPT_ifso_version_EQ)) ---------------- Can we make this required to be specified instead of defaulting please? I think that it is more clear. ================ Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1651 + if (Args.hasArg(OPT_ifso_version_EQ)) + if(Args.getLastArgValue(OPT_ifso_version_EQ) == "experimental-ifo-elf-v1") + Opts.ProgramAction = frontend::GenerateIfsoObjYamlExpV1; ---------------- Please use a covered `StringSwitch` here. Since this is in the frontend, I think that we should have a test that the driver diagnoses invalid values. ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:164 +class IFSOFunctionsConsumer : public ASTConsumer { + CompilerInstance &Instance; ---------------- This does not belong here. Please extract it into a separate file, much like the serialization is extracted into a separate file. ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:179 + }; + using MangledSymbols = std::map<const NamedDecl *, MangledSymbol>; + ---------------- Hmm, I wonder if `DenseSet` is more appropriate here. Is there some reason that I am overlooking for supporting duplicate entries? In fact, since the key is a pointer, you could even do a `DenseMap`. ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:182 + template <typename T> + bool WriteNamedDecl(const NamedDecl *ND, MangledSymbols &Symbols, int RDO) { + if (!isa<T>(ND)) ---------------- I'm not sure I like this very much. You are templating over a type a function that needs to walk the hierarchy for `NamedDecl`. This is going to re-emit the function and without LTO cause a large `.text` contribution for something that should really just be a jump table. Would it not be possible to use a covered switch on the `kindof()` in the `NamedDecl`. The covering should ensure that future changes would allow us to find this site. ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:189 + return true; + if (ND->getVisibility() != DefaultVisibility) + return true; ---------------- I understand this, but, it may be nice to have a comment explaining why we only consider default visibility (hidden visibility interfaces cannot be seen by consumers, and protected mode visibility in ELF does not participate in linking, only loading). ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:197 + return true; + index::CodegenNameGenerator CGNameGen(ND->getASTContext()); + std::string MangledName = CGNameGen.getName(ND); ---------------- Newline before this would be nice. ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:199 + std::string MangledName = CGNameGen.getName(ND); + uint8_t Type = llvm::ELF::STT_FUNC; + uint8_t Binding = llvm::ELF::STB_GLOBAL; ---------------- This is confusing, can you rewrite this as, dropping L204-205: ``` uint8_t Type = isa<VarDecl>(ND) ? llvm::ELF::STT_OBJECT : llvm::ELF::STT_FUNC; ``` ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:203 + ND->isWeakImported()) + Binding = llvm::ELF::STB_WEAK; + if (isa<VarDecl>(ND)) ---------------- Again, I think that this is slightly confusing, and would prefer a ternary. ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:207 + + Symbols.insert(std::pair<const NamedDecl *, MangledSymbol>( + ND, MangledSymbol(MangledName, Type, Binding))); ---------------- Can you not use a universal initialiser? In the case you cannot, `std::make_pair` would be nicer to use for the type deduction. ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:214 + ND->dump(); + } + return true; ---------------- debugging left over? ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:219 + template <typename T> + bool HandleSomeDecl(const NamedDecl *ND, MangledSymbols &Symbols, int RDO) { + if (!isa<T>(ND)) ---------------- Similar to the previous case, I am slightly worried about the size contributions here. Why not a covered switch? ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:222 + return false; + for (auto *I : cast<T>(ND)->decls()) + HandleNamedDecl(dyn_cast<NamedDecl>(I), Symbols, RDO); ---------------- `const auto *D`? This also makes very little sense to the casual reader. This cast is very confusing. There is no reason to assume that an arbitrary `NamedDecl` is a `DeclContext`. It is only made even more confusing by the fact that the decl that is retrieved is a pointer - since that is an implementation detail of the `decl_iterator` in the `decl_range` returned by `decls` which is implicitly dereferenced by the range based for loop. Making the type explicit by the means of the covered switch and an explicit cast to the `DeclContext` should help in clearing that up. ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:223 + for (auto *I : cast<T>(ND)->decls()) + HandleNamedDecl(dyn_cast<NamedDecl>(I), Symbols, RDO); + return true; ---------------- Hmm, do we have a guarantee that the decl is named? Could you not hit a `_Static_assert` or `static_assert` in this traversal? Or if it is a C++ context, an `extern "C"` block? What about `#pragma comment(...)`? Please add test cases for these. ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:227 + + template <typename T> + bool HandleSomeDeclSpec(const NamedDecl *ND, MangledSymbols &Symbols, ---------------- Again, the templating here worries me. ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:232 + return false; + for (auto *I : cast<T>(ND)->specializations()) + HandleNamedDecl(dyn_cast<NamedDecl>(I), Symbols, RDO); ---------------- Hmm, what guarantee do we have that the templated type is a template type? ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:254 + return true; + // While IFSOs are in the development stage, it's probably best to catch + // anything that's not a VarDecl or Template/FunctionDecl. ---------------- Please put this in an `LLVM_DEBUG`. ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:269 + bool VisitNamedDecl(NamedDecl *ND) { + if (auto *FD = dyn_cast<FunctionDecl>(ND)) + (FD->isLateTemplateParsed() ? LateParsedDecls : NamedDecls) ---------------- const? I think that this might be easier to parse for the reader as: ``` if (const auto *FD = dyn_cast<FunctionDecl>(ND)) if (FD->isLateTemplateParsed()) return LateParsedDecls.insert(FD).first != LateParsedDecls.end(); else return NamedDecls.insert(FD).first != NamedDecls.end(); if (const auto *VD = dyn_cast<ValueDecl>(ND)) return ValueDecls.insert(VD).first != ValueDecls.end(); return NamedDecls.insert(ND) != NamedDecls.end(); ``` ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:292 + if (Instance.getLangOpts().DelayedTemplateParsing) { + clang::Sema &sema = Instance.getSema(); + for (const auto *FD : v.LateParsedDecls) { ---------------- `S` is the traditional initialism. ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:297 + sema.LateTemplateParser(sema.OpaqueParser, LPT); + HandleNamedDecl(FD, Symbols, (FromTU | IsLate)); + } ---------------- Typo of `||`? The field is boolean not a mask. ================ Comment at: clang/lib/Frontend/FrontendActions.cpp:341 + << "\nSymbols:\n"; + for (auto E : Symbols) { + const MangledSymbol &Symbol = E.second; ---------------- `const auto &E`? I wish that we had structured decomposition already. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60974/new/ https://reviews.llvm.org/D60974 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits