This is an archive of the discontinued LLVM Phabricator instance.

Handle errors in expansion of response files
ClosedPublic

Authored by sepavloff on Oct 17 2022, 9:06 AM.

Details

Summary

Previously an error raised during an expansion of response files (including
configuration files) was ignored and only the fact of its presence was
reported to the user with generic error messages. This made it difficult to
analyze problems. For example, if a configuration file tried to read an
inexistent file, the error message said that 'configuration file cannot
be found', which is wrong and misleading.

This change enhances handling errors in the expansion so that users
could get more informative error messages.

Diff Detail

Event Timeline

sepavloff created this revision.Oct 17 2022, 9:06 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
sepavloff requested review of this revision.Oct 17 2022, 9:06 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 17 2022, 9:06 AM

Make UTF-16 string properly aligned

rovka added inline comments.Oct 21 2022, 6:25 AM
clang/test/Driver/config-file-errs.c
16

I think you might need to use the same trick as this test to get the test to pass on both Windows and Linux (since they use different capitalization).

rovka added a comment.Oct 21 2022, 7:46 AM

Nice tests! :) I have one microscopic nit and one question (because I don't know an awful lot about config files). Thanks for working on this.

llvm/lib/Support/CommandLine.cpp
1188

Why do you need to add !InConfigFile here and below?

1208

Nit: Why reverse this branch? The code around it continues early, so it's easier to read if this continues early too.

mgorny added inline comments.Oct 21 2022, 7:48 AM
llvm/lib/Support/CommandLine.cpp
1209

Also, I think you can use consume_front() here.

sepavloff updated this revision to Diff 469957.Oct 22 2022, 9:17 PM

Small changes proposed by reviewers

sepavloff added inline comments.Oct 22 2022, 9:30 PM
llvm/lib/Support/CommandLine.cpp
1188

It is not necessary, because now configuration file is expanded with the flag RelativeNames set. It however seems more correct to check for InConfigFile also, hopefully it make code a bit clearly.

1208

You are right. Initially this patch was combined with D136354, where additional checks was added. I changed this code to be more natural.

1209

Yes, changed code accordingly.

mgorny added inline comments.Oct 22 2022, 10:37 PM
clang/lib/Driver/Driver.cpp
1042–1044

Either I'm missing something or you're not using EC.

1054

Not necessarily a goal for this patch but wouldn't this check fit better in readConfigFile()?

clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
67–69

The braces seem unnecessary.

sepavloff updated this revision to Diff 469978.Oct 23 2022, 5:34 AM

Updated patch

sepavloff added inline comments.Oct 23 2022, 5:36 AM
clang/lib/Driver/Driver.cpp
1042–1044

You are right, this is remnants of previous implementation. Thanks for the catch.

1054

Yes, it makes sense. Moved.

clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
67–69

Fixed.

mgorny added inline comments.Oct 28 2022, 12:37 AM
clang/unittests/Driver/ToolChainTest.cpp
604–606

Wouldn't it be possible to use std::string, or maybe even std::array<char, ...> here? I think it'd be less error-prone.

sepavloff added inline comments.Oct 28 2022, 3:15 AM
clang/unittests/Driver/ToolChainTest.cpp
604–606

Nor std::string neither std::array<char,N> provide custom alignment.

UTF-16 string is constructed here from array of chars to avoid problems with endianness. It consists of 2-byte elements and is expected to be aligned on 2-byte boundary. Array of chars is aligned on byte and sometimes the test failed due to invalid alignment.

So we have to use bare pointer to have guaranteed alignment.

mgorny accepted this revision.Oct 28 2022, 5:19 AM

LGTM

clang/unittests/Driver/ToolChainTest.cpp
604–606

Ah. Could you leave a comment to make the alignment requirements clear? I'm a bit surprised that we have to meet alignment requirements when "reading" a file.

This revision is now accepted and ready to land.Oct 28 2022, 5:19 AM
This revision was automatically updated to reflect the committed changes.
uabelho added a subscriber: uabelho.EditedNov 1 2022, 3:23 AM

Hello,

I'm wondering if this is really working as expected/intended.

With this patch, if I do

clang empty.c @atfileesc.txt

on an empty file empty.c and atfileesc.txt containing just

-I @name-with-at/

then I get the following error

cannot open file: /repo/uabelho/main-github/llvm/atfileesc.txt
Program aborted due to an unhandled Error:
cannot open file: /repo/uabelho/main-github/llvm/atfileesc.txt
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: build-all/bin/clang empty.c @atfileesc.txt -dry-run
 #0 0x0000000002fc9873 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (build-all/bin/clang+0x2fc9873)
 #1 0x0000000002fc758e llvm::sys::RunSignalHandlers() (build-all/bin/clang+0x2fc758e)
 #2 0x0000000002fc9bf6 SignalHandler(int) (build-all/bin/clang+0x2fc9bf6)
 #3 0x00007f678e7ae630 __restore_rt (/lib64/libpthread.so.0+0xf630)
 #4 0x00007f678bef5387 raise (/lib64/libc.so.6+0x36387)
 #5 0x00007f678bef6a78 abort (/lib64/libc.so.6+0x37a78)
 #6 0x0000000002f39d00 llvm::Error::fatalUncheckedError() const (build-all/bin/clang+0x2f39d00)
 #7 0x00000000009d5e3a clang_main(int, char**) (build-all/bin/clang+0x9d5e3a)
 #8 0x00007f678bee1555 __libc_start_main (/lib64/libc.so.6+0x22555)
 #9 0x00000000009d233b _start (build-all/bin/clang+0x9d233b)
Abort (core dumped)

Note that the file it is complaining about indeed exists:

-> ls -l /repo/uabelho/main-github/llvm/atfileesc.txt
-rw-r--r-- 1 uabelho eusers 18 Nov  1 11:15 /repo/uabelho/main-github/llvm/atfileesc.txt

and as I wrote above it contains

-> cat /repo/uabelho/main-github/llvm/atfileesc.txt
-I @name-with-at/
bjope added a subscriber: bjope.Nov 1 2022, 4:06 AM

Thank you for reporting the issue. It was fixed in fdab9f1203ee.

Thank you for reporting the issue. It was fixed in fdab9f1203ee.

Thanks!