[PATCH] D60974: Clang IFSO driver action.
Saleem Abdulrasool via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue May 7 08:54:25 PDT 2019
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
More information about the cfe-commits
mailing list