[PATCH] D34304: Allow CompilerInvocations to generate .d files.
klimek accepted this revision.
klimek added a comment.
This revision is now accepted and ready to land.
LG, thx for bearing with me, this looks great.
(and sorry if I didn't send this earlier, just noticed I forgot to hit submit
:( )
Comment at: lib/Tooling/ArgumentsAdjusters.cpp:66
+ (Arg == "-MD") || (Arg == "-MMD")) {
+// Output is specified as -MX foo. Skip the next argument also.
+++i;
s/ also/, too/?
https://reviews.llvm.org/D34304
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34304: Allow CompilerInvocations to generate .d files.
saugustine updated this revision to Diff 104307.
saugustine added a comment.
Rework this patch to use argument adjusters. It turns out that
the call to newInvocation from ClangFuzzer has a very limited
set of hard-coded arguments, so I don't think it is necessary
to do the hand-adjusting there. Any new callers would be on their
own.
https://reviews.llvm.org/D34304
Files:
include/clang/Tooling/ArgumentsAdjusters.h
include/clang/Tooling/Tooling.h
lib/Tooling/ArgumentsAdjusters.cpp
lib/Tooling/Tooling.cpp
Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -100,7 +100,6 @@
*Diagnostics);
Invocation->getFrontendOpts().DisableFree = false;
Invocation->getCodeGenOpts().DisableFree = false;
- Invocation->getDependencyOutputOpts() = DependencyOutputOptions();
return Invocation;
}
@@ -510,7 +509,8 @@
std::unique_ptr buildASTFromCodeWithArgs(
const Twine &Code, const std::vector &Args,
const Twine &FileName, const Twine &ToolName,
-std::shared_ptr PCHContainerOps) {
+std::shared_ptr PCHContainerOps,
+ArgumentsAdjuster Adjuster) {
SmallString<16> FileNameStorage;
StringRef FileNameRef = FileName.toNullTerminatedStringRef(FileNameStorage);
@@ -523,8 +523,10 @@
OverlayFileSystem->pushOverlay(InMemoryFileSystem);
llvm::IntrusiveRefCntPtr Files(
new FileManager(FileSystemOptions(), OverlayFileSystem));
- ToolInvocation Invocation(getSyntaxOnlyToolArgs(ToolName, Args, FileNameRef),
-&Action, Files.get(), std::move(PCHContainerOps));
+
+ ToolInvocation Invocation(
+ getSyntaxOnlyToolArgs(ToolName, Adjuster(Args, FileNameRef), FileNameRef),
+ &Action, Files.get(), std::move(PCHContainerOps));
SmallString<1024> CodeStorage;
InMemoryFileSystem->addFile(FileNameRef, 0,
Index: lib/Tooling/ArgumentsAdjusters.cpp
===
--- lib/Tooling/ArgumentsAdjusters.cpp
+++ lib/Tooling/ArgumentsAdjusters.cpp
@@ -51,6 +51,26 @@
};
}
+ArgumentsAdjuster getClangStripDependencyFileAdjuster() {
+ return [](const CommandLineArguments &Args, StringRef /*unused*/) {
+CommandLineArguments AdjustedArgs;
+for (size_t i = 0, e = Args.size(); i < e; ++i) {
+ StringRef Arg = Args[i];
+ // All dependency-file options begin with -M. These include -MM,
+ // -MF, -MG, -MP, -MT, -MQ, -MD, and -MMD.
+ if (!Arg.startswith("-M"))
+AdjustedArgs.push_back(Args[i]);
+
+ if ((Arg == "-MF") || (Arg == "-MT") || (Arg == "-MQ") ||
+ (Arg == "-MD") || (Arg == "-MMD")) {
+// Output is specified as -MX foo. Skip the next argument also.
+++i;
+ }
+}
+return AdjustedArgs;
+ };
+}
+
ArgumentsAdjuster getInsertArgumentAdjuster(const CommandLineArguments &Extra,
ArgumentInsertPosition Pos) {
return [Extra, Pos](const CommandLineArguments &Args, StringRef /*unused*/) {
@@ -83,4 +103,3 @@
} // end namespace tooling
} // end namespace clang
-
Index: include/clang/Tooling/Tooling.h
===
--- include/clang/Tooling/Tooling.h
+++ include/clang/Tooling/Tooling.h
@@ -202,12 +202,15 @@
/// \param PCHContainerOps The PCHContainerOperations for loading and creating
/// clang modules.
///
+/// \param Adjuster A function to filter the command line arguments as specified.
+///
/// \return The resulting AST or null if an error occurred.
std::unique_ptr buildASTFromCodeWithArgs(
const Twine &Code, const std::vector &Args,
const Twine &FileName = "input.cc", const Twine &ToolName = "clang-tool",
std::shared_ptr PCHContainerOps =
-std::make_shared());
+ std::make_shared(),
+ArgumentsAdjuster Adjuster = getClangStripDependencyFileAdjuster());
/// \brief Utility to run a FrontendAction in a single clang invocation.
class ToolInvocation {
Index: include/clang/Tooling/ArgumentsAdjusters.h
===
--- include/clang/Tooling/ArgumentsAdjusters.h
+++ include/clang/Tooling/ArgumentsAdjusters.h
@@ -44,6 +44,10 @@
/// arguments.
ArgumentsAdjuster getClangStripOutputAdjuster();
+/// \brief Gets an argument adjuster which removes dependency-file
+/// related command line arguments.
+ArgumentsAdjuster getClangStripDependencyFileAdjuster();
+
enum class ArgumentInsertPosition { BEGIN, END };
/// \brief Gets an argument adjuster which inserts \p Extra arguments in the
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34304: Allow CompilerInvocations to generate .d files.
klimek added a comment. In https://reviews.llvm.org/D34304#788080, @saugustine wrote: > In https://reviews.llvm.org/D34304#787699, @klimek wrote: > > > I mean, arguments need to be adjusted before converting to ArgStringList > > and calling newInvocation? I'm not sure I fully understand the problem, can > > you elaborate? > > > This gets back to why the original patch plumbed the boolean all the way down > to newInvocation. > > newInvocation is the function that actually discards the dependency file > options--today unconditionally. Correct, and it shouldn't. > But there is code that calls newInvocation directly (ClangFuzzer is one), > without going through a higher-level API. So I can't adjust the arguments at > a higher level and still preserve the old behavior. Can't the call sites that use it themselves fix the arguments themselves? We should be able to provide a convenience wrapper for those use cases, I'd guess they all look basically the same? > Unfortunately, newInvocation's argument list type is incompatible with > ArgumentAdjusters, so something else will need to be done. What do you > recommend? I'd have expected something like: 1. create ArgumentAdjuster that filters out deps file creation args 2. make that the default in the higher level APIs that already use ArgumentAdjuster 3. for each call site that uses the lower level API, look into what data they have (probably just argv from main?) and create some nice ArgumentAdjuster wrapper so they can use the default argument adjuster with a 1 line change https://reviews.llvm.org/D34304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34304: Allow CompilerInvocations to generate .d files.
saugustine added a comment. In https://reviews.llvm.org/D34304#787699, @klimek wrote: > I mean, arguments need to be adjusted before converting to ArgStringList and > calling newInvocation? I'm not sure I fully understand the problem, can you > elaborate? This gets back to why the original patch plumbed the boolean all the way down to newInvocation. newInvocation is the function that actually discards the dependency file options--today unconditionally. But there is code that calls newInvocation directly (ClangFuzzer is one), without going through a higher-level API. So I can't adjust the arguments at a higher level and still preserve the old behavior. Unfortunately, newInvocation's argument list type is incompatible with ArgumentAdjusters, so something else will need to be done. What do you recommend? https://reviews.llvm.org/D34304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34304: Allow CompilerInvocations to generate .d files.
klimek added a comment. I mean, arguments need to be adjusted before converting to ArgStringList and calling newInvocation? I'm not sure I fully understand the problem, can you elaborate? https://reviews.llvm.org/D34304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34304: Allow CompilerInvocations to generate .d files.
saugustine added a comment. > Actually, now that I figured out you mean ArgumentAdjusters, I am making > progress. Unfortunately, ArgumentAdjusters only work on vector, and while ToolInvocation::Invocation takes its arguments in that form, tooling::newInvocation (which returns a CompilerInvocation) takes a SmallVector, so we either need to change one side's interface, or write two ArgumentAdjusters, but with similar semantics. What is your preferred solution? https://reviews.llvm.org/D34304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34304: Allow CompilerInvocations to generate .d files.
saugustine added a comment. In https://reviews.llvm.org/D34304#785984, @saugustine wrote: > In https://reviews.llvm.org/D34304#785083, @klimek wrote: > > > I think it's cleaner, because it uses a concept we already have (argument > > adapters). > > > Will you point me to an example of these. Google is coming up empty. Actually, now that I figured out you mean ArgumentAdjusters, I am making progress. https://reviews.llvm.org/D34304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34304: Allow CompilerInvocations to generate .d files.
saugustine added a comment. In https://reviews.llvm.org/D34304#785083, @klimek wrote: > I think it's cleaner, because it uses a concept we already have (argument > adapters). Will you point me to an example of these. Google is coming up empty. https://reviews.llvm.org/D34304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34304: Allow CompilerInvocations to generate .d files.
klimek added a comment. In https://reviews.llvm.org/D34304#784496, @saugustine wrote: > In https://reviews.llvm.org/D34304#783675, @klimek wrote: > > > I think a better way might be to generally leave dependency options alone, > > add a default argument adapter to filter out all deps related flags, and > > allow users to add their own argument adapters that don't do that. > > > This argument adapter would have to be passed down in a similar way, no? > > buildASTFromCodeWithArgs, toolIinvocation::Run, and newInvocation are all > entry-points that would need this behavior, and all are called by themselves > in one place or another. I think it's cleaner, because it uses a concept we already have (argument adapters). > I'm happy to do that, as it is quite a bit more flexible, but it doesn't seem > any cleaner. https://reviews.llvm.org/D34304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34304: Allow CompilerInvocations to generate .d files.
saugustine added a subscriber: klimek. saugustine added a comment. In https://reviews.llvm.org/D34304#783675, @klimek wrote: > I think a better way might be to generally leave dependency options alone, > add a default argument adapter to filter out all deps related flags, and > allow users to add their own argument adapters that don't do that. This argument adapter would have to be passed down in a similar way, no? buildASTFromCodeWithArgs, toolIinvocation::Run, and newInvocation are all entry-points that would need this behavior, and all are called by themselves in one place or another. I'm happy to do that, as it is quite a bit more flexible, but it doesn't seem any cleaner. https://reviews.llvm.org/D34304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34304: Allow CompilerInvocations to generate .d files.
klimek added a comment. I think a better way might be to generally leave dependency options alone, add a default argument adapter to filter out all deps related flags, and allow users to add their own argument adapters that don't do that. https://reviews.llvm.org/D34304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34304: Allow CompilerInvocations to generate .d files.
echristo edited reviewers, added: bkramer; removed: echristo. echristo added a comment. Going to let Ben review this. I'd rather not pass the bool down and he might know a way to avoid that here by knowing the code more :) https://reviews.llvm.org/D34304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
