This is an archive of the discontinued LLVM Phabricator instance.

Fix the error message with -fbasic-block-sections=list=<filename>
ClosedPublic

Authored by tmsriram on Oct 15 2020, 2:58 PM.

Details

Summary

Let the front-end handle the case where the file doesnt exist. The driver only checks if the option syntax is right.

Diff Detail

Event Timeline

tmsriram created this revision.Oct 15 2020, 2:58 PM
tmsriram requested review of this revision.Oct 15 2020, 2:58 PM

Oops, let me fix the diff first.

tmsriram updated this revision to Diff 298484.Oct 15 2020, 3:07 PM

Fix the diff.

Hmm - are other file existence checks done in the driver? (eg: for source files themselves) I'd have assumed not - would have thought we'd let the filename propagate down until it's actually opened and then failed there. (means fewer error paths - since the latter's necessary anyway, and removes the chance of racey file creation/deletion/etc producing different behavior)

Hmm - are other file existence checks done in the driver? (eg: for source files themselves) I'd have assumed not - would have thought we'd let the filename propagate down until it's actually opened and then failed there. (means fewer error paths - since the latter's necessary anyway, and removes the chance of racey file creation/deletion/etc producing different behavior)

Yep, I see a couple, -fprofile-generate, -fbuild-session-timestamp and frewrite-map-file. I see a plus in failing early and really dont have an opinion about the racey behavior.

Hmm - are other file existence checks done in the driver? (eg: for source files themselves) I'd have assumed not - would have thought we'd let the filename propagate down until it's actually opened and then failed there. (means fewer error paths - since the latter's necessary anyway, and removes the chance of racey file creation/deletion/etc producing different behavior)

Yep, I see a couple, -fprofile-generate, -fbuild-session-timestamp and frewrite-map-file.

Curious.

I see a plus in failing early

Any particular benefits you're thinking of? If the code later on will have to have some error path for file not existing anyway & could print an error message there.

and really dont have an opinion about the racey behavior.

tmsriram updated this revision to Diff 298496.Oct 15 2020, 3:55 PM
tmsriram edited the summary of this revision. (Show Details)

Based on dblaikie's comment, remove the driver check for file exists completely as the front-end will do this anyways and report an appropriate error upon failure.

Based on dblaikie's comment, remove the driver check for file exists completely as the front-end will do this anyways and report an appropriate error upon failure.

Is there a test that needs to be removed that was testing the driver behavior? And is there a test testing the underlying/compiler ("frontend") error behavior?

rahmanl added inline comments.Oct 15 2020, 5:48 PM
clang/lib/Driver/ToolChains/Clang.cpp
4927

I think this string checking code here is error prone. I see some functionality duplicated in BackendUtil.cpp (like CodeGenOpts.BBSections.substr(5)). Is there a reason we're not using a more structured format (struct with enum) to represent the different choices for Options.BBSections?

Based on dblaikie's comment, remove the driver check for file exists completely as the front-end will do this anyways and report an appropriate error upon failure.

Is there a test that needs to be removed that was testing the driver behavior? And is there a test testing the underlying/compiler ("frontend") error behavior?

There was not test added to check the error behavior. I can add one now.

clang/lib/Driver/ToolChains/Clang.cpp
4927

This is the driver where we are actually consuming the string input. This is converted to an enum in BackendUtil.cpp. Since strlen("list=") is 5 I don't see a problem as we are explicitly checking that Val starts with this. Do you prefer anything else?

rahmanl added inline comments.Oct 15 2020, 9:17 PM
clang/lib/Driver/ToolChains/Clang.cpp
4927

Ah got it.
So BackendUtil converts the string into Options.BBSections and Options.BBSectionsFuncListBuf and returns the error if the file cannot be read. No need to return the error here.

tmsriram updated this revision to Diff 298735.Oct 16 2020, 1:14 PM
tmsriram marked 2 inline comments as done.

Test the error messages in the driver and the frontend.

In the driver, test for invalid value and in the front end test for non-existent file.

MaskRay added inline comments.
clang/test/Frontend/fbasic-block-sections.c
1 ↗(On Diff #298735)

This writes a temporary file in the current working directory, which is not supported by our internal lit runner. -o /dev/null

MaskRay added inline comments.Oct 16 2020, 2:04 PM
clang/test/Frontend/fbasic-block-sections.c
1 ↗(On Diff #298735)

Emm. I am unsure you need this test. Doesn't test/CodeGen/basic-block-sections.c capture the test coverage?

BTW, if you want to avoid output, you can also use -emit-llvm-only. But it should not be used here.

tmsriram added inline comments.Oct 16 2020, 2:13 PM
clang/test/Frontend/fbasic-block-sections.c
1 ↗(On Diff #298735)

Acknowledged. I added this as an after thought myself and was not sure about this. Why don't I just get rid of it and just keep the error check.

tmsriram updated this revision to Diff 298758.Oct 16 2020, 2:30 PM

Remove checks of valid options in frontend as codegen already does that.

dblaikie accepted this revision.Oct 19 2020, 9:35 PM

Generally looks OK to me - wouldn't mind if the test were rolled into wherever the rest of the fbasic-block-sections= testing is done (looks like the error message is emitted in CodeGen, rather than Frontend, so might make it easier for future developers to find associated tests if they're in the directory that matches what they're testing) but don't feel especially strongly.

@MaskRay and @rahmanl might have further feedback.

This revision is now accepted and ready to land.Oct 19 2020, 9:35 PM
MaskRay accepted this revision.Oct 19 2020, 10:24 PM

Generally looks OK to me - wouldn't mind if the test were rolled into wherever the rest of the fbasic-block-sections= testing is done (looks like the error message is emitted in CodeGen, rather than Frontend, so might make it easier for future developers to find associated tests if they're in the directory that matches what they're testing) but don't feel especially strongly.

@MaskRay and @rahmanl might have further feedback.

+1 for folding the CC1 negative test into test/CodeGen/fbasic-block-sections.c

tmsriram marked 2 inline comments as done.Oct 20 2020, 4:31 PM
tmsriram updated this revision to Diff 299511.Oct 20 2020, 4:36 PM

Address reviewer comments, move the negative check into CodeGen.

thakis added a subscriber: thakis.Oct 20 2020, 6:19 PM

Looks like this breaks tests on windows: http://45.33.8.238/win/26253/step_7.txt

Please take a look and revert for now if it takes a while to fix.

A git bisect run blamed this patch for breaking the ability to build with a read-only source tree. Can we revert this or make a quick fix?

FAIL: Clang :: CodeGen/basic-block-sections.c (3385 of 26774)
******************** TEST 'Clang :: CodeGen/basic-block-sections.c' FAILED ********************
Script:
--
: 'RUN: at line 3';   /tmp/_update_lc/r/bin/clang -cc1 -internal-isystem /tmp/_update_lc/r/lib/clang/12.0.0/include -nostdsysteminc -triple x86_64 -S -o - < /home/dave/ro_s/lp/clang/test/CodeGen/basic-block-sections.c | /tmp/_update_lc/r/bin/FileCheck /home/dave/ro_s/lp/clang/test/CodeGen/basic-block-sections.c --check-prefix=PLAIN
: 'RUN: at line 4';   /tmp/_update_lc/r/bin/clang -cc1 -internal-isystem /tmp/_update_lc/r/lib/clang/12.0.0/include -nostdsysteminc -triple x86_64 -S -fbasic-block-sections=all -fbasic-block-sections=none -o - < /home/dave/ro_s/lp/clang/test/CodeGen/basic-block-sections.c | /tmp/_update_lc/r/bin/FileCheck /home/dave/ro_s/lp/clang/test/CodeGen/basic-block-sections.c --check-prefix=PLAIN
: 'RUN: at line 6';   /tmp/_update_lc/r/bin/clang -cc1 -internal-isystem /tmp/_update_lc/r/lib/clang/12.0.0/include -nostdsysteminc -triple x86_64 -S -fbasic-block-sections=all -o - < /home/dave/ro_s/lp/clang/test/CodeGen/basic-block-sections.c | /tmp/_update_lc/r/bin/FileCheck /home/dave/ro_s/lp/clang/test/CodeGen/basic-block-sections.c --check-prefix=BB_WORLD --check-prefix=BB_ALL
: 'RUN: at line 7';   /tmp/_update_lc/r/bin/clang -cc1 -internal-isystem /tmp/_update_lc/r/lib/clang/12.0.0/include -nostdsysteminc -triple x86_64 -S -fbasic-block-sections=list=/home/dave/ro_s/lp/clang/test/CodeGen/Inputs/basic-block-sections.funcnames -o - < /home/dave/ro_s/lp/clang/test/CodeGen/basic-block-sections.c | /tmp/_update_lc/r/bin/FileCheck /home/dave/ro_s/lp/clang/test/CodeGen/basic-block-sections.c --check-prefix=BB_WORLD --check-prefix=BB_LIST
: 'RUN: at line 8';   /tmp/_update_lc/r/bin/clang -cc1 -internal-isystem /tmp/_update_lc/r/lib/clang/12.0.0/include -nostdsysteminc -triple x86_64 -S -fbasic-block-sections=all -funique-basic-block-section-names -o - < /home/dave/ro_s/lp/clang/test/CodeGen/basic-block-sections.c | /tmp/_update_lc/r/bin/FileCheck /home/dave/ro_s/lp/clang/test/CodeGen/basic-block-sections.c --check-prefix=UNIQUE
: 'RUN: at line 9';   not /tmp/_update_lc/r/bin/clang -cc1 -internal-isystem /tmp/_update_lc/r/lib/clang/12.0.0/include -nostdsysteminc -fbasic-block-sections=list= -emit-obj /home/dave/ro_s/lp/clang/test/CodeGen/basic-block-sections.c 2>&1 | /tmp/_update_lc/r/bin/FileCheck /home/dave/ro_s/lp/clang/test/CodeGen/basic-block-sections.c --check-prefix=ERROR
--
Exit Code: 1

Command Output (stderr):
--
/home/dave/ro_s/lp/clang/test/CodeGen/basic-block-sections.c:42:11: error: ERROR: expected string not found in input
// ERROR: error: unable to load basic block sections function list: 'No such file or directory'
          ^
<stdin>:1:1: note: scanning from here
error: unable to open output file '': 'Read-only file system'
^

Input file: <stdin>
Check file: /home/dave/ro_s/lp/clang/test/CodeGen/basic-block-sections.c

-dump-input=help explains the following input dump.

Input was:
<<<<<<
          1: error: unable to open output file '': 'Read-only file system'
check:42     X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
          2: 1 error generated.
check:42     ~~~~~~~~~~~~~~~~~~
>>>>>>

--

********************
********************
Failed Tests (1):
  Clang :: CodeGen/basic-block-sections.c


Testing Time: 76.20s
  Unsupported      :   341
  Passed           : 26404
  Expectedly Failed:    28
  Failed           :     1

I think I fixed it. Please verify: 87f6de72bcd346bbbf468e9f9a0e9d1bbf0630a9

Thanks - if that's unblocked you, great!

@tmsriram could you check on this? I'm surprised this test isn't failing - I can't see any CHECK lines for the "ERROR" prefix used by the error test (& FileCheck usually fails if it finds no relevant CHECK lines... so I'm not sure what's happening there).

Also: I'd expect a failing compilation not to write the file out? (I'd expect it to be writing to a temporary file, then moving the file into position once the compilation has finished successfully, so there's never a half-written file, etc - in case the compiler crashes) so I wonder what's going on here. Could you check what the behavior is/why it's trying to write a file even when there's an error? (I could be wrong on my assumptions/understanding here - perhaps this is true of other error paths too, that they do try to write the output file and maybe delete it after instead (though that wouldn't be crash-resilient))

@tmsriram ping on the follow-up here

I checked chmod -w a.dwo; clang -cc1 -debug-info-kind=limited
-dwarf-version=4 -split-dwarf-file a.dwo -split-dwarf-output a.dwo
-emit-obj -o - split-debug-output.c
which suppresses the output, so -fbasic-block-sections=list= should
follow the convention as well.

Sent https://reviews.llvm.org/D90815

It is rare to report an error in BackendUtil.cpp . So I checked the
other Diags.Report instance and noticed that -split-dwarf-file a.dwo
-split-dwarf-output a.dwo (when a.dwo is not writable) suppresses the
output. So there is no reason that -fbasic-block-sections=list= should
not follow the convention.

Yes, that's right. I think most errors are caught in Sema. There "if
there is an error, the output should be suppressed" should be a
consensus.
For some auxiliary files it was a bit fuzzy to me. So I checked .dwo
which is a similar auxiliary output - the output is suppressed with an
error - so it becomes clear to me that -fbasic-block-sections=list=
should behave consistently.