Fix assembler error when -g and -gdwarf-* is passed with -fno-integrated-as.

[garvitgupta08]

Summary

Fix assembler error when -g and -gdwarf-* is passed with
-fno-integrated-as.

D136309 and D136707 are 2 differentials that pass -g and -gdwarf-* to
assembler when -fno-integrated-as is used. Due to this we started seeing
below assembler error with GNU binutils version 2.34 and below. This
error is not shown by binutils version 2.36 and later.

Error: file number 1 already allocated"

This is because the debug info generated at the source code level is conflicting
with the debug info generated by assembler as mentioned in the gcc bug report -
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=35925

This patch solves the above failure by passing -g and -gdwarf-* flags to
assembler only when the source code is assembly, otherwise just generate the
debug info at the source code level.

The above error can be reproduced through below testcase-
(clang from main, Ubuntu 16.04).

$ cat test.cpp
int foo()
{
        int i=6;
        do --i; while (!(i%3));
        do {} while (!(i%5));
        return 0;
}
$ clang++ test.cpp --target=arm-linux-gnueabi -c -fno-integrated-as -gdwarf-4 -O2 -fno-finite-loops --gcc-toolchain=<path/to/GNU/assembler/for/arm>
/tmp/C06127-9298ce.s: Assembler messages:
/tmp/C06127-9298ce.s:56: Error: file number 1 already allocated
clang++: error: assembler command failed with exit code 1 (use -v to see invocation)

[MaskRay]

Due to this we started seeing assembler errors with certain .c and .cpp files -
"Error: file number 1 already allocated"

What are the certain .c and .cpp files? The behavior is correct for the following two commands.

clang --target=arm-linux-gnueabihf -fno-integrated-as -c a.cc
clang --target=arm-linux-gnueabihf -fno-integrated-as -S a.c

(I'll be out for most of the next 4 weeks and will spend little time on reviews.)

[efriedma]

Following reproduces for me (clang from main, Ubuntu 16.04).

$ cat test.cpp
int foo()
{
        int i=6;
        do --i; while (!(i%3));
        do {} while (!(i%5));
        return 0;
}
$ clang++ test.cpp -c -fno-integrated-as -gdwarf-4 -O2 -fno-finite-loops
/tmp/test-f97496.s: Assembler messages:
/tmp/test-f97496.s:18: Error: file number 1 already allocated
clang++: error: assembler command failed with exit code 1 (use -v to see invocation)

I think it triggers when the assembly file contains code before the first ".file" directive.

[nickdesaulniers]

Isn't this only an issue with ancient versions of GNU as? Older than 2.16?

[garvitgupta08]

Added the testcase in the commit message itself. Thanks Eli for providing the same!

[nickdesaulniers]

Rather than create new test files, shouldn't this modify the same tests modified in D136309 and D136707?

[garvitgupta08]

In D145726#4190316, @nickdesaulniers wrote:

Isn't this only an issue with ancient versions of GNU as? Older than 2.16?

I am able to reproduce this issue with 4.9.0 GNU as.

[nickdesaulniers]

In D145726#4190341, @garvitgupta08 wrote:

In D145726#4190316, @nickdesaulniers wrote:

Isn't this only an issue with ancient versions of GNU as? Older than 2.16?

I am able to reproduce this issue with 4.9.0 GNU as.

That version number looks like a GCC version number.

GAS should be 2.39 or close by. Please run as --version.

[nickdesaulniers]

clang/lib/Driver/ToolChains/Gnu.cpp

978–986

Isn't this potentially going to add -gdwarf- repeatedly if there's many inputs?

Wouldn't it be better to scan the inputs to see if there's any .S or .s files, then add the flags once?

[efriedma]

In D145726#4190341, @garvitgupta08 wrote:

In D145726#4190316, @nickdesaulniers wrote:

Isn't this only an issue with ancient versions of GNU as? Older than 2.16?

I am able to reproduce this issue with 4.9.0 GNU as.

binutils has a separate version from gcc.

Tried the following file with binutils on godbolt; apparently binutils 2.34 and earlier reject, binutils 2.36 and later accept. So we're not talking about the most recent binutils, but 2.34 isn't exactly ancient.

nop
.file 1 "asdf"

[garvitgupta08]

Addressed the comments. Using gcc_forward.c for checking c extension.

[garvitgupta08]

clang/lib/Driver/ToolChains/Gnu.cpp

978–986

Let me know if this is fine.

[nickdesaulniers]

The description needs to be rewritten to elucidate that this is purely to support old versions of the GNU assembler still in use by various Linux LTS distros. This problem does not exist for folks using up to date tooling. I would like to see specific GNU assembler version details in the commit description as well.

clang/lib/Driver/ToolChains/Gnu.cpp

978–986

I don't think the current implementation addresses my point. Having CmdArgs.push_back be called in a loop on the number of inputs will potentially add the arg repeatedly.

I think you should simply check if the InputType is asm or asm-with-cpp in the loop, potentially setting a boolean scoped outside the loop. Then, after the loop, decide whether to add the cmd arg.

[nickdesaulniers]

clang/lib/Driver/ToolChains/Gnu.cpp

978–986

Specifically, if llvm::any_of the inputs are asm or asm-with-cpp, then we might want to modify the command line flags passed to the external assembler.

We don't want to pass additional flags per input.

[garvitgupta08]

In D145726#4194142, @nickdesaulniers wrote:

The description needs to be rewritten to elucidate that this is purely to support old versions of the GNU assembler still in use by various Linux LTS distros. This problem does not exist for folks using up to date tooling. I would like to see specific GNU assembler version details in the commit description as well.

Done

[garvitgupta08]

clang/lib/Driver/ToolChains/Gnu.cpp

978–986

Done

[nickdesaulniers]

clang/lib/Driver/ToolChains/Gnu.cpp

976–985

Thinking about this more, does the issue still exist if the user passed .c and .s/.S files together?

i.e. $ clang ... -fno-integrated-as -gdwarf-4 foo.s main.c?

clang/test/Driver/as-options.cpp

1

please chmod 644 this file; 755 is what I'd expect for a directory.

[garvitgupta08]

clang/lib/Driver/ToolChains/Gnu.cpp

976–985

Yes, the error will still be thrown for c/cpp files.

[nickdesaulniers]

clang/lib/Driver/ToolChains/Gnu.cpp

976–985

So this patch is an incomplete fix then? Is there somewhere else we can move this logic then so that it's only applied for individual files and not multiple inputs?

[garvitgupta08]

clang/lib/Driver/ToolChains/Gnu.cpp

976–985

No. I misunderstood your question. I thought you were asking what will happen in case of multiple inputs without this fix. I answered according to this.

The current patch fixes the error in case of multiple inputs as well. No error will be thrown for c/cpp files. I hope this answers your original question.

[garvitgupta08]

I do not have the commit access, can you commit on my behalf @nickdesaulniers

[nickdesaulniers]

In D145726#4225082, @garvitgupta08 wrote:

I do not have the commit access, can you commit on my behalf @nickdesaulniers

Sure thing; I'll test it out with some Linux kernel builds, too. I'll try to get that done today by EOD; thanks again for the patch!

[nickdesaulniers]

@garvitgupta08 what's your email address so that I may attribute your authorship correctly when committing on your behalf?

[garvitgupta08]

In D145726#4228216, @nickdesaulniers wrote:

@garvitgupta08 what's your email address so that I may attribute your authorship correctly when committing on your behalf?

[e-mail]. Thanks for committing!.

[MaskRay]

Please update the commit message and comment about what binutils versions reject the construct.

clang/test/Driver/gcc_forward.c

49

No newline at end of file

Please fix

[nickdesaulniers]

clang/lib/Driver/ToolChains/Gnu.cpp

980–981

The following input causes clang to crash:

$ clang -fno-integrated-as -c -x assembler-with-cpp /dev/null
clang: /android0/llvm-project/llvm/include/llvm/ADT/StringRef.h:597: llvm::StringRef llvm::StringRef::drop_front(size_t) const: Assertion `size() >= N && "Dropping more elements than exist"' failed.

Please fix that and add a test case for /dev/null (i.e. inputs for which no extension exists).

[garvitgupta08]

In D145726#4228406, @MaskRay wrote:

Please update the commit message and comment about what binutils versions reject the construct.

I already added this - "Due to this we started seeing
below assembler error with GNU binutils version 2.34 and below. This
error is not shown by binutils version 2.36 and later."

clang/lib/Driver/ToolChains/Gnu.cpp

980–981

For Inputs without extension, will there always be a -x flag?

[nickdesaulniers]

clang/lib/Driver/ToolChains/Gnu.cpp

980–981

I assume the input can have any extension or no extension. Maybe find where in clang we decide we don't know the extension, and emulate a similar approach?

For example, clang foo.xyz will complain that we don't know the source language. clang -x c foo.xyz should shut up the warning if foo.xyz is a valid c file.

clang/lib/Driver/ToolChains/Gnu.cpp

   CmdArgs.push_back("-o");
   CmdArgs.push_back(Output.getFilename());
 
-  for (const auto &II : Inputs)
-    CmdArgs.push_back(II.getFilename());
-
-  if (Arg *A = Args.getLastArg(options::OPT_g_Flag, options::OPT_gN_Group,
-                               options::OPT_gdwarf_2, options::OPT_gdwarf_3,
-                               options::OPT_gdwarf_4, options::OPT_gdwarf_5,
-                               options::OPT_gdwarf))
-    if (!A->getOption().matches(options::OPT_g0)) {
-      Args.AddLastArg(CmdArgs, options::OPT_g_Flag);
 
-      unsigned DwarfVersion = getDwarfVersion(getToolChain(), Args);
-      CmdArgs.push_back(Args.MakeArgString("-gdwarf-" + Twine(DwarfVersion)));
-    }
+  bool IsInputTyAsm = false;
+  for (const auto &II : Inputs) {
+    CmdArgs.push_back(II.getFilename());
+    StringRef BaseInput = StringRef(II.getBaseInput());
+    types::ID InputType = types::lookupTypeForExtension(
+        llvm::sys::path::extension(BaseInput).drop_front());
+    if (InputType == types::TY_Asm || InputType == types::TY_PP_Asm)
+      IsInputTyAsm = true;
+  }
+
+  if (IsInputTyAsm)
+    if (Arg *A = Args.getLastArg(options::OPT_g_Flag, options::OPT_gN_Group,
+                                 options::OPT_gdwarf_2, options::OPT_gdwarf_3,
+                                 options::OPT_gdwarf_4, options::OPT_gdwarf_5,
+                                 options::OPT_gdwarf))
+      if (!A->getOption().matches(options::OPT_g0)) {
+        Args.AddLastArg(CmdArgs, options::OPT_g_Flag);
+        unsigned DwarfVersion = getDwarfVersion(getToolChain(), Args);
+        CmdArgs.push_back(Args.MakeArgString("-gdwarf-" + Twine(DwarfVersion)));
+      }
 
   const char *Exec =
       Args.MakeArgString(getToolChain().GetProgramPath(DefaultAssembler));

clang/test/Driver/as-options.cpp

+// Test that -g and -gdwarf-* are not passed through to GAS.
+// RUN: %clang --target=arm-linux-gnueabi -fno-integrated-as -g -c %s -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=DEBUG %s
+// RUN: %clang --target=arm-linux-gnueabi -fno-integrated-as -gdwarf-4 -c %s -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=DEBUG %s
+// RUN: %clang --target=arm-linux-gnueabi -fno-integrated-as -gdwarf-4 -g -c %s -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=DEBUG %s
+// RUN: %clang --target=arm-linux-gnueabi -fno-integrated-as -g \
+// RUN:   -fdebug-default-version=4 -c %s -### 2>&1 | FileCheck --check-prefix=DEBUG %s
+// DEBUG-NOT: "-g"
+// DEBUG-NOT: "-gdwarf-4"
 No newline at end of file

clang/test/Driver/gcc_forward.c

 // CHECK-NOT: "-Wall"
 // CHECK-NOT: "-Wdocumentation"
 // CHECK: "-o" "a.out"
+//
+// Test that -g and -gdwarf-* are not passed through to GAS.
+// RUN: %clang --target=arm-linux-gnueabi -fno-integrated-as -g -c %s -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=DEBUG %s
+// RUN: %clang --target=arm-linux-gnueabi -fno-integrated-as -gdwarf-4 -c %s -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=DEBUG %s
+// RUN: %clang --target=arm-linux-gnueabi -fno-integrated-as -gdwarf-4 -g -c %s -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=DEBUG %s
+// RUN: %clang --target=arm-linux-gnueabi -fno-integrated-as -g \
+// RUN:   -fdebug-default-version=4 -c %s -### 2>&1 | FileCheck --check-prefix=DEBUG %s
+// DEBUG-NOT: "-g"
+// DEBUG-NOT: "-gdwarf-4"
 No newline at end of file