[test] Use host platform specific error message substitution in lit tests

[abhina.sreeskantharajan]

Summary

This patch uses the errno python library to print out the correct error messages instead of hardcoding the error message per platform.

[jhenderson]

If it works, this is a much nicer solution, thanks. I didn't know about the errno library. Do you need me to verify it on Windows?

[abhina.sreeskantharajan]

In D97472#2587671, @jhenderson wrote:

If it works, this is a much nicer solution, thanks. I didn't know about the errno library. Do you need me to verify it on Windows?

Yes, that would be great! Thanks. I've already verified that it works on z/OS and Linux on Power.

[ASDenysPetrov]

In D95246#2587526, @abhina.sreeskantharajan wrote:

Thanks, I'm also not sure if the second way is possible. I had another idea but I will need @ASDenysPetrov to test it out.
Does the following python program print out the correct error string for you?

import errno
import os
print ( os.strerror(errno.ENOENT))

If so, we can use this as the error message instead of hardcoding it per platform.

This is my output:

>>> import errno
>>> import os
>>> print(os.strerror(errno.ENOENT))
No such file or directory
>>>

I've tested this patch. Yes, I confirm, it works! Well done!

[abhina.sreeskantharajan]

I'm not sure why CI is failing, so I've tried rebasing my patch.
I have also fixed this testcase that fails on MSYS, llvm/test/tools/llvm-elfabi/fail-file-write-windows.test.

[ASDenysPetrov]

In D97472#2588079, @abhina.sreeskantharajan wrote:

I'm not sure why CI is failing, so I've tried rebasing my patch.
I have also fixed this testcase that fails on MSYS, llvm/test/tools/llvm-elfabi/fail-file-write-windows.test.

LGTM

[jhenderson]

Bad news: this doesn't work on Windows. I've dug into why, and unfortunately it's a bizarre difference in behaviour between two different ways of getting the same error message from an error code on Windows. Given the following mini C++ program:

int main()
{
  printf("%s\n", strerror(EINVAL));
  printf("%s\n", std::make_error_code(std::errc(EINVAL)).message().c_str());
  return 0;
}

The output is:

Invalid argument
invalid argument

Note that the first uses an upper-case first letter, and the second a lower-case letter. Python, which is written in C will be using the first of these lines to get its error message. However, for the messages produced by LLVM, we are using the C++ std::error_code version, which produes the other version. This is obviously a slight problem...!

I wonder if we could use the old approach but have the substitution use a regex still (i.e. something like {{[Nn]}}? That would satisfy our use cases at least. It may not work though. I'm not sure if FileCheck variables can be regex patterns or not.

[abhina.sreeskantharajan]

Thanks for testing it out. Since os.strerror does not emit the correct message on Windows we have to go back to hardcoding the error message for Windows. I had the same thought of passing a regex but FileCheck does not allow it. I currently check the host_cxx to see if MSYS is in the path because the library path is not available in the config.

I see there is a check for msys bash in the same file that looks like this:

if sys.platform in ['win32'] and not self.use_lit_shell:
    # Don't pass dosish path separator to msys bash.exe.

Is this a potential alternative to using host_cxx?

[ASDenysPetrov]

@abhina.sreeskantharajan
I really prefer the patch you made. Can we just ignore the case as a workaround for Windows in such case?

[abhina.sreeskantharajan]

Ignore case on Windows

[abhina.sreeskantharajan]

Fix CI: Ignore extra --ignore-case added by this commit https://github.com/llvm/llvm-project/commit/3b4a0adec2720675571585a0a0dc366fe8e75853#diff-363dbbce88704f02b7f5c133bc7c6b3b0c550571c10bf049497fb8c425a4b89f to fix the same error on Windows.

[ASDenysPetrov]

@abhina.sreeskantharajan
I've checked it. This patch works for me. I'm ready to approve.
@jhenderson could you check it again as well?

[jhenderson]

I haven't physically checked this (I expect it would work), but I don't think we want to add --ignore-case, in case there are other parts of the check that will be checked where case is important. For example, if the filename was part of the error message, we want it to match exactly the case of the file, as doing anything else can result in ambiguity and therefore risk for a bug to be hidden (most Windows filesystems are case insensitive, but it is possible for them to be case sensitive). Hiding the --ignore-case option behind a substitution would also be a potential for serious confusion, in my opinion, e.g. if a user explicitly wants to ignore the case themselves for other parts of their check.

I've subscribed @thopre and @jdenny, who both work on FileCheck. Maybe they'll have some other ideas. If not, I think we have to detect the C++ standard library being used somehow (or at least the compiler path, which is probably a good-enough proxy).

[thopre]

In D97472#2599694, @jhenderson wrote:

I haven't physically checked this (I expect it would work), but I don't think we want to add --ignore-case, in case there are other parts of the check that will be checked where case is important. For example, if the filename was part of the error message, we want it to match exactly the case of the file, as doing anything else can result in ambiguity and therefore risk for a bug to be hidden (most Windows filesystems are case insensitive, but it is possible for them to be case sensitive). Hiding the --ignore-case option behind a substitution would also be a potential for serious confusion, in my opinion, e.g. if a user explicitly wants to ignore the case themselves for other parts of their check.

I've subscribed @thopre and @jdenny, who both work on FileCheck. Maybe they'll have some other ideas. If not, I think we have to detect the C++ standard library being used somehow (or at least the compiler path, which is probably a good-enough proxy).

I cannot think of a solution on FileCheck (the regex are allowed when matching/defining a variable, not when using it) but I don't think it's the right place to address this issue. That would require anyone using the substitution to be aware of the casing difference. I think the best approach is to lower the casing of the first letter of error message on Windows. The current code already checks for win32 platform so we don't lose more generality than we what we already have. We could even abstract that behind a function (e.g. llvm_strerror).

[abhina.sreeskantharajan]

In D97472#2599773, @thopre wrote:

In D97472#2599694, @jhenderson wrote:

I haven't physically checked this (I expect it would work), but I don't think we want to add --ignore-case, in case there are other parts of the check that will be checked where case is important. For example, if the filename was part of the error message, we want it to match exactly the case of the file, as doing anything else can result in ambiguity and therefore risk for a bug to be hidden (most Windows filesystems are case insensitive, but it is possible for them to be case sensitive). Hiding the --ignore-case option behind a substitution would also be a potential for serious confusion, in my opinion, e.g. if a user explicitly wants to ignore the case themselves for other parts of their check.

I've subscribed @thopre and @jdenny, who both work on FileCheck. Maybe they'll have some other ideas. If not, I think we have to detect the C++ standard library being used somehow (or at least the compiler path, which is probably a good-enough proxy).

I cannot think of a solution on FileCheck (the regex are allowed when matching/defining a variable, not when using it) but I don't think it's the right place to address this issue. That would require anyone using the substitution to be aware of the casing difference. I think the best approach is to lower the casing of the first letter of error message on Windows. The current code already checks for win32 platform so we don't lose more generality than we what we already have. We could even abstract that behind a function (e.g. llvm_strerror).

I agree adding ignore-case may not be ideal especially because we can only specify it once. If that's the case, is my previous diff on this patch better? We check

if sys.platform == 'win32' and 'MSYS' not in host_cxx

[thopre]

In D97472#2599930, @abhina.sreeskantharajan wrote:

In D97472#2599773, @thopre wrote:

In D97472#2599694, @jhenderson wrote:

I haven't physically checked this (I expect it would work), but I don't think we want to add --ignore-case, in case there are other parts of the check that will be checked where case is important. For example, if the filename was part of the error message, we want it to match exactly the case of the file, as doing anything else can result in ambiguity and therefore risk for a bug to be hidden (most Windows filesystems are case insensitive, but it is possible for them to be case sensitive). Hiding the --ignore-case option behind a substitution would also be a potential for serious confusion, in my opinion, e.g. if a user explicitly wants to ignore the case themselves for other parts of their check.

I've subscribed @thopre and @jdenny, who both work on FileCheck. Maybe they'll have some other ideas. If not, I think we have to detect the C++ standard library being used somehow (or at least the compiler path, which is probably a good-enough proxy).

I cannot think of a solution on FileCheck (the regex are allowed when matching/defining a variable, not when using it) but I don't think it's the right place to address this issue. That would require anyone using the substitution to be aware of the casing difference. I think the best approach is to lower the casing of the first letter of error message on Windows. The current code already checks for win32 platform so we don't lose more generality than we what we already have. We could even abstract that behind a function (e.g. llvm_strerror).

I agree adding ignore-case may not be ideal especially because we can only specify it once. If that's the case, is my previous diff on this patch better? We check

if sys.platform == 'win32' and 'MSYS' not in host_cxx

If you mean version 3 (aka https://reviews.llvm.org/D97472?id=326671) of your diff then I personally prefer yes. It's not as elegant but it's less surprising.

[abhina.sreeskantharajan]

Revert to previous commit. Set host_cxx to empty string if not set.

[jdenny]

I may be lost. Why is --ignore-case still needed in llvm/test/tools/yaml2obj/ELF/DWARF/debug-gnu-pubnames.yaml? I thought the special case for win32 was supposed to handle that.

[ASDenysPetrov]

LGTM. Let it be the most balanced solution.

[abhina.sreeskantharajan]

In D97472#2600526, @jdenny wrote:

I may be lost. Why is --ignore-case still needed in llvm/test/tools/yaml2obj/ELF/DWARF/debug-gnu-pubnames.yaml? I thought the special case for win32 was supposed to handle that.

I missed this testcase in my old patch. I noticed it because it started to fail when I tried to add --ignore-case as a solution. I can fix it in this patch as well if desired or open a separate patch for it after this one.

[jdenny]

In D97472#2600955, @abhina.sreeskantharajan wrote:

In D97472#2600526, @jdenny wrote:

I may be lost. Why is --ignore-case still needed in llvm/test/tools/yaml2obj/ELF/DWARF/debug-gnu-pubnames.yaml? I thought the special case for win32 was supposed to handle that.

I missed this testcase in my old patch. I noticed it because it started to fail when I tried to add --ignore-case as a solution. I can fix it in this patch as well if desired or open a separate patch for it after this one.

Either way is fine by me. I was just concerned I had misunderstood what was happening.

Based on @jhenderson's comments, it sounds like C++ code could still call strerror(EINVAL) on windows. It doesn't look like that would be handled by this patch, but I suppose we can cross that bridge if we actually come to it.

Of the solutions I've seen, the current one seems best.

[abhina.sreeskantharajan]

Sorry @jdenny, I misunderstood your comment. I removed the old --ignore-case workaround and fixed a similar lit test.

[abhina.sreeskantharajan]

Correct patch.

[abhina.sreeskantharajan]

@jhenderson does the solution in this patch look acceptable to you?

[jhenderson]

The approach basically looks fine. I haven't had a chance to verify it today. I'll try to find time tomorrow if I can.

In D97472#2601108, @jdenny wrote:

Based on @jhenderson's comments, it sounds like C++ code could still call strerror(EINVAL) on windows. It doesn't look like that would be handled by this patch, but I suppose we can cross that bridge if we actually come to it.

Yes, I expect that this could be a potential problem, but it's probably solved by saying "don't do that", probably in the coding standards? Perhaps worth an RFC on the mailing list to see if others have opinions. There are a limited number of existing cases that actually do call strerror in LLVM, but I'm guessing none of them are tested, at least not outside of a Linux environment.

llvm/utils/lit/lit/llvm/config.py

350

Add a comment here explaining why we have to treat windows behaviour separately.

[abhina.sreeskantharajan]

Thanks! Adding comment

[jhenderson]

LGTM. I've tested this on a small sample of tests that use these substitutions, using my Windows machine, and the tests pass.

llvm/test/tools/llvm-elfabi/fail-file-write-windows.test

 ## Test failing to write output file on windows platform.
 
 # REQUIRES: system-windows
 # RUN: touch %t.TestFile
 # RUN: chmod 400 %t.TestFile
-# RUN: not llvm-elfabi %s --output-target=elf64-little %t.TestFile 2>&1 | FileCheck %s --check-prefix=ERR
+# RUN: not llvm-elfabi %s --output-target=elf64-little %t.TestFile 2>&1 | FileCheck -DMSG=%errc_EACCES %s --check-prefix=ERR
 # RUN: chmod 777 %t.TestFile
 # RUN: rm -rf %t.TestFile
 
 --- !tapi-tbe
 TbeVersion: 1.0
 Arch: AArch64
 Symbols: {}
 ...
 
-# ERR: error: permission denied
+# ERR: error: [[MSG]]

llvm/utils/lit/lit/llvm/config.py

 import itertools
 import os
 import platform
 import re
 import subprocess
 import sys
+import errno
 
 import lit.util
 from lit.llvm.subst import FindTool
 from lit.llvm.subst import ToolSubst
 
 lit_path_displayed = False
 
 class LLVMConfig(object):
def add_tool_substitutions(self, tools, search_dirs=None):
                 return False
 
             substitutions.append((subst_key, tool_pipe + command))
 
         self.config.substitutions.extend(substitutions)
         return True
 
     def add_err_msg_substitutions(self):
-        if (sys.platform == 'zos'):
-            self.config.substitutions.append(('%errc_ENOENT', '\'EDC5129I No such file or directory.\''))
-            self.config.substitutions.append(('%errc_EISDIR', '\'EDC5123I Is a directory.\''))
-            self.config.substitutions.append(('%errc_EINVAL', '\'EDC5121I Invalid argument.\''))
-            self.config.substitutions.append(('%errc_EACCES', '\'EDC5111I Permission denied.\''))
-        elif (sys.platform == 'win32'):
-            self.config.substitutions.append(('%errc_ENOENT', '\'no such file or directory\''))
-            self.config.substitutions.append(('%errc_EISDIR', '\'is a directory\''))
-            self.config.substitutions.append(('%errc_EINVAL', '\'invalid argument\''))
-            self.config.substitutions.append(('%errc_EACCES', '\'permission denied\''))
-        else:
-            self.config.substitutions.append(('%errc_ENOENT', '\'No such file or directory\''))
-            self.config.substitutions.append(('%errc_EISDIR', '\'Is a directory\''))
-            self.config.substitutions.append(('%errc_EINVAL', '\'Invalid argument\''))
-            self.config.substitutions.append(('%errc_EACCES', '\'Permission denied\''))
+        ignore_case = ' --ignore-case' if sys.platform == 'win32' else ''
+        self.config.substitutions.append(('%errc_ENOENT', '\'' + os.strerror(errno.ENOENT) + '\'' + ignore_case))
+        self.config.substitutions.append(('%errc_EISDIR', '\'' + os.strerror(errno.EISDIR) + '\'' + ignore_case))
+        self.config.substitutions.append(('%errc_EINVAL', '\'' + os.strerror(errno.EINVAL) + '\'' + ignore_case))
+        self.config.substitutions.append(('%errc_EACCES', '\'' + os.strerror(errno.EACCES) + '\'' + ignore_case))
 
     def use_default_substitutions(self):
         tool_patterns = [
             ToolSubst('FileCheck', unresolved='fatal'),
             # Handle these specially as they are strings searched for during testing.
             ToolSubst(r'\| \bcount\b', command=FindTool(
                 'count'), verbatim=True, unresolved='fatal'),
             ToolSubst(r'\| \bnot\b', command=FindTool('not'), verbatim=True, unresolved='fatal')]