Fix lack of cc1 flag in llvmcmd sections when assertions are enabled

[aidengrossman]

Summary

Currently when assertions are enabled, the cc1 flag is not
inserted into the llvmcmd section of object files with embedded
bitcode. This deviates from the normal behavior where this is
the first flag that is inserted. This error stems from incorrect
use of the function generateCC1CommandLine() which requires
manually adding in the -cc1 flag which is currently not done.

[jansvoboda11]

Thanks for the fix! Left one suggestion.

clang/lib/Frontend/CompilerInvocation.cpp

4549

This will shift all generated arguments in the vector. Could you do something like this instead?

Args.push_back("-cc1");
Invocation.generateCC1CommandLine(Args, SA);

I think generateCC1CommandLine() appends to Args, so this should be safe to do.

[aidengrossman]

Moved insertion of cc1 flag to prevent shifting entire array

[aidengrossman]

clang/lib/Frontend/CompilerInvocation.cpp

4549

Fixed. Thanks for the suggestion. Just tested it and everything looks good. For some reason when I was working on it this morning I didn't think it would be that easy.

[jansvoboda11]

The code change looks good to me, could you also include a test case for this?

[aidengrossman]

Added two unit tests to ensure functionality is correct in both
the assertion case and the default case. Should be sufficient
to ensure correct functionality without utilizing regression tests.
However, if regression tests are also desired, let me know and I can
add one in.

[jansvoboda11]

Thanks for adding the tests. They are failing in CI though, and I don't think they're entirely correct.

My understanding is that the goal is to make sure Clang doesn't drop the -cc1 flag when embedding command-lines into object files.

During normal compilation, the arguments given by the driver to CompilerInvocation::CreateFromArgs() already begin with -cc1, so I suggest setting up the tests to do so as well. We could use {"-cc1", "-round-trip-args"} in the first test case and {"-cc1", "-no-round-trip-args"} in the second.

We probably want to verify that the CompilerInvocation member used to store the command-line looks correctly. My guess is that the bitcode is using CodeGenOptions::CommandLineArgs, so I suggest checking that instead of GeneratedArgs in the final assertion.

(I feel like GeneratedArgs are an implementation detail in this particular test case. Moreover, the vector will be empty in the current revision, since you don't call Invocation.generateCC1CommandLine(GeneratedArgs, *this) like the surrounding test cases do.)

[aidengrossman]

Rebased against main and adjusted unit tests to check directly
for the cc1 flag in CmdArgs which is directly consumed by the
bitcode writer while creating the .llvmcmd section. Also changed
the flags to better match what is already getting passed in.

[jansvoboda11]

LGTM, thanks for the patch!

[aidengrossman]

Thanks for the review and your suggestions. Do you mind pushing this commit? I don't currently have commit access to LLVM. Thanks.

[mtrofin]

In D130620#3686357, @aidengrossman wrote:

Thanks for the review and your suggestions. Do you mind pushing this commit? I don't currently have commit access to LLVM. Thanks.

I can do it.

clang/lib/Frontend/CompilerInvocation.cpp

bool CompilerInvocation::CreateFromArgs(CompilerInvocation &Invocation,
   CompilerInvocation DummyInvocation;
 
   return RoundTrip(
       [](CompilerInvocation &Invocation, ArrayRef<const char *> CommandLineArgs,
          DiagnosticsEngine &Diags, const char *Argv0) {
         return CreateFromArgsImpl(Invocation, CommandLineArgs, Diags, Argv0);
       },
       [](CompilerInvocation &Invocation, SmallVectorImpl<const char *> &Args,
-         StringAllocator SA) { Invocation.generateCC1CommandLine(Args, SA); },
+         StringAllocator SA) {
+        Args.push_back("-cc1");
+        Invocation.generateCC1CommandLine(Args, SA);
+      },
       Invocation, DummyInvocation, CommandLineArgs, Diags, Argv0);
 }
 
 std::string CompilerInvocation::getModuleHash() const {
   // FIXME: Consider using SHA1 instead of MD5.
   llvm::HashBuilder<llvm::MD5, llvm::support::endianness::native> HBuilder;
 
   // Note: For QoI reasons, the things we use as a hash here should all be

clang/unittests/Frontend/CompilerInvocationTest.cpp

 #include "gtest/gtest.h"
 
 using namespace llvm;
 using namespace clang;
 
 using ::testing::Contains;
 using ::testing::HasSubstr;
 using ::testing::StrEq;
+using ::testing::StartsWith;
 
 namespace {
 class CommandLineTest : public ::testing::Test {
 public:
   IntrusiveRefCntPtr<DiagnosticsEngine> Diags;
   SmallVector<const char *, 32> GeneratedArgs;
   SmallVector<std::string, 32> GeneratedArgsStorage;
   CompilerInvocation Invocation;
TEST_F(CommandLineTest, BoolOptionDefaultTrueSingleFlagPresent) {
   ASSERT_TRUE(CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags));
   ASSERT_FALSE(Invocation.getFrontendOpts().UseTemporary);
 
   Invocation.generateCC1CommandLine(GeneratedArgs, *this);
 
   ASSERT_THAT(GeneratedArgs, Contains(StrEq("-fno-temp-file")));
 }
 
+TEST_F(CommandLineTest, CC1FlagPresentWhenDoingRoundTrip) {
+  const char *Args[] = {"-cc1", "-round-trip-args"};
+
+  ASSERT_TRUE(CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags));
+
+  ASSERT_THAT(std::string(Invocation.getCodeGenOpts().CmdArgs.begin(),
+                          Invocation.getCodeGenOpts().CmdArgs.end()),
+              StartsWith("-cc1"));
+}
+
+TEST_F(CommandLineTest, CC1FlagPresentWhenNotDoingRoundTrip) {
+  const char *Args[] = {"-cc1", "-no-round-trip-args"};
+
+  ASSERT_TRUE(CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags));
+
+  ASSERT_THAT(std::string(Invocation.getCodeGenOpts().CmdArgs.begin(),
+                          Invocation.getCodeGenOpts().CmdArgs.end()),
+              StartsWith("-cc1"));
+}
+
 TEST_F(CommandLineTest, BoolOptionDefaultTrueSingleFlagUnknownPresent) {
   const char *Args[] = {"-ftemp-file"};
 
   // Driver-only flag.
   ASSERT_FALSE(CompilerInvocation::CreateFromArgs(Invocation, Args, *Diags));
   ASSERT_TRUE(Invocation.getFrontendOpts().UseTemporary);
 }