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

[tmsriram]

Summary

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

[tmsriram]

Oops, let me fix the diff first.

[tmsriram]

Fix the diff.

[dblaikie]

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)

[tmsriram]

In D89500#2333472, @dblaikie wrote:

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.

[dblaikie]

In D89500#2333496, @tmsriram wrote:

In D89500#2333472, @dblaikie wrote:

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]

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.

[dblaikie]

In D89500#2333518, @tmsriram wrote:

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]

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?

[tmsriram]

In D89500#2333547, @dblaikie wrote:

In D89500#2333518, @tmsriram wrote:

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]

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]

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]

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]

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]

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]

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

[dblaikie]

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.

[MaskRay]

In D89500#2340779, @dblaikie wrote:

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]

Address reviewer comments, move the negative check into CodeGen.

[thakis]

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.

[davezarzycki]

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

[davezarzycki]

I think I fixed it. Please verify: 87f6de72bcd346bbbf468e9f9a0e9d1bbf0630a9

[dblaikie]

In D89500#2344234, @davezarzycki wrote:

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

[dblaikie]

@tmsriram ping on the follow-up here

[MaskRay]

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

[MaskRay]

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.

[MaskRay]

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.

clang/lib/Driver/ToolChains/Clang.cpp

if (Args.hasFlag(options::OPT_ffunction_sections,
                    options::OPT_fno_function_sections, UseSeparateSections)) {
     CmdArgs.push_back("-ffunction-sections");
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_fbasic_block_sections_EQ)) {
     if (Triple.isX86() && Triple.isOSBinFormatELF()) {
       StringRef Val = A->getValue();
       if (Val != "all" && Val != "labels" && Val != "none" &&
-          !(Val.startswith("list=") && llvm::sys::fs::exists(Val.substr(5))))
+          !Val.startswith("list="))
         D.Diag(diag::err_drv_invalid_value)
             << A->getAsString(Args) << A->getValue();
       else
         A->render(Args, CmdArgs);
     } else {
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << A->getAsString(Args) << TripleStr;
     }

clang/test/CodeGen/basic-block-sections.c

 // REQUIRES: x86-registered-target
 
 // RUN: %clang_cc1 -triple x86_64 -S -o - < %s | FileCheck %s --check-prefix=PLAIN
 // RUN: %clang_cc1 -triple x86_64 -S -fbasic-block-sections=all -fbasic-block-sections=none -o - < %s | FileCheck %s --check-prefix=PLAIN
 
 // RUN: %clang_cc1 -triple x86_64 -S -fbasic-block-sections=all -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_ALL
 // RUN: %clang_cc1 -triple x86_64 -S -fbasic-block-sections=list=%S/Inputs/basic-block-sections.funcnames -o - < %s | FileCheck %s --check-prefix=BB_WORLD --check-prefix=BB_LIST
 // RUN: %clang_cc1 -triple x86_64 -S -fbasic-block-sections=all -funique-basic-block-section-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
+// RUN: not %clang_cc1 -fbasic-block-sections=list= -emit-obj %s 2>&1 | FileCheck %s --check-prefix=ERROR
 
 int world(int a) {
   if (a > 10)
     return 10;
   else if (a > 5)
     return 5;
   else
     return 0;

 // BB_ALL: .section .text.another,"ax",@progbits,unique
 // BB_ALL: another.1:
 // BB_LIST-NOT: .section .text.another,"ax",@progbits,unique
 // BB_LIST: another:
 // BB_LIST-NOT: another.1:
 //
 // UNIQUE: .section .text.world.world.1,
 // UNIQUE: .section .text.another.another.1,
+// ERROR: error:  unable to load basic block sections function list: 'No such file or directory'

clang/test/Driver/fbasic-block-sections.c

 // RUN: %clang -### -target x86_64 -fbasic-block-sections=none %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-NONE %s
 // RUN: %clang -### -target x86_64 -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-ALL %s
 // RUN: %clang -### -target x86_64 -fbasic-block-sections=list=%s %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LIST %s
 // RUN: %clang -### -target x86_64 -fbasic-block-sections=labels %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
 // RUN: not %clang -c -target arm-unknown-linux -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
 // RUN: not %clang -c -target x86_64-apple-darwin10 -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=alll %s -S 2>&1 | FileCheck -check-prefix=CHECK-INVALID-VALUE %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=list %s -S 2>&1 | FileCheck -check-prefix=CHECK-INVALID-VALUE %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=list= %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-NULL-LIST %s
 //
 // CHECK-OPT-NONE:   "-fbasic-block-sections=none"
 // CHECK-OPT-ALL:    "-fbasic-block-sections=all"
 // CHECK-OPT-LIST:   "-fbasic-block-sections={{[^ ]*}}fbasic-block-sections.c"
 // CHECK-OPT-LABELS: "-fbasic-block-sections=labels"
 // CHECK-TRIPLE:     error: unsupported option '-fbasic-block-sections=all' for target
+// CHECK-INVALID-VALUE: error: invalid value {{[^ ]*}} in '-fbasic-block-sections={{.*}}'
+// CHECK-OPT-NULL-LIST: "-fbasic-block-sections=list="