This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by garvitgupta08 on Mar 9 2023, 1:17 PM.

Details

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)

Diff Detail

Event Timeline

garvitgupta08 created this revision.Mar 9 2023, 1:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2023, 1:17 PM
garvitgupta08 requested review of this revision.Mar 9 2023, 1:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2023, 1:17 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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.)

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.

garvitgupta08 edited the summary of this revision. (Show Details)Mar 13 2023, 11:23 AM
garvitgupta08 edited the summary of this revision. (Show Details)Mar 13 2023, 11:24 AM

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

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

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

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.

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.

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?

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"

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

clang/lib/Driver/ToolChains/Gnu.cpp
978–986

Let me know if this is fine.

nickdesaulniers requested changes to this revision.Mar 14 2023, 12:23 PM

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.

This revision now requires changes to proceed.Mar 14 2023, 12:23 PM
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 marked an inline comment as not done.
garvitgupta08 edited the summary of this revision. (Show Details)

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 added inline comments.Mar 21 2023, 3:10 PM
clang/lib/Driver/ToolChains/Gnu.cpp
978–986

Done

clang/lib/Driver/ToolChains/Gnu.cpp
976–996

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
2

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

garvitgupta08 added inline comments.
clang/lib/Driver/ToolChains/Gnu.cpp
976–996

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

clang/lib/Driver/ToolChains/Gnu.cpp
976–996

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 added inline comments.Mar 27 2023, 11:07 AM
clang/lib/Driver/ToolChains/Gnu.cpp
976–996

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.

nickdesaulniers accepted this revision.Mar 27 2023, 11:16 AM
This revision is now accepted and ready to land.Mar 27 2023, 11:16 AM

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

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!

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

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

quic_garvgupt@quicinc.com. Thanks for committing!.

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 requested changes to this revision.Mar 28 2023, 1:27 PM
nickdesaulniers added inline comments.
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).

This revision now requires changes to proceed.Mar 28 2023, 1:27 PM

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?

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.