Handle intrin files on win32 with gcc

[kenneth-olwing]

Hi all,

This is effectively a new fix for the issue #18025, merged in https://github.com/Perl/perl5/pull/18026.

As it happens, that fix can't ever have worked as I understand it as tests are made against a file basename only, but the variable contains full pathnames. Further, it only addresses two possibilities, but there are more headers that exhibit the behavior that it tries to fix.

It may not have been noticed since it depends on the order of the files, coming as keys from a hash in a non-determinate fashion, and since in many such cases the 'right' files happens to have been #include'd first, and then there simply is no error. Other reasons are that the error is only noticeable if you eyeball the build log, since the code doesn't report back the fact that the gcc run has failed - nor even if it does, the build system doesn't stop.

No need to actually build, just running the Errno_pm.PL script is enough to eventually hit it. In my experience at least once per 10 tries, but typically more; here are some sample runs (using a portable Strawberry 5.32.1 on a Win11 machine):

C:\ws\git\perl5\ext\Errno>perl Errno_pm.PL

C:\ws\git\perl5\ext\Errno>perl Errno_pm.PL

C:\ws\git\perl5\ext\Errno>perl Errno_pm.PL

C:\ws\git\perl5\ext\Errno>perl Errno_pm.PL

C:\ws\git\perl5\ext\Errno>perl Errno_pm.PL

C:\ws\git\perl5\ext\Errno>perl Errno_pm.PL

C:\ws\git\perl5\ext\Errno>perl Errno_pm.PL

C:\ws\git\perl5\ext\Errno>perl Errno_pm.PL
In file included from includes.c:1:
c:/ken1/slask/straw/SP-5.32.1.1/c/lib/gcc/x86_64-w64-mingw32/8.3.0/include/clwbintrin.h:25:3: error: #error "Never use <clwbintrin.h> directly; include <x86intrin.h> instead."
 # error "Never use <clwbintrin.h> directly; include <x86intrin.h> instead."
   ^~~~~

C:\ws\git\perl5\ext\Errno>perl Errno_pm.PL

C:\ws\git\perl5\ext\Errno>perl Errno_pm.PL
In file included from includes.c:1:
c:/ken1/slask/straw/SP-5.32.1.1/c/lib/gcc/x86_64-w64-mingw32/8.3.0/include/adxintrin.h:25:3: error: #error "Never use <adxintrin.h> directly; include <x86intrin.h> instead."
 # error "Never use <adxintrin.h> directly; include <x86intrin.h> instead."
   ^~~~~
In file included from includes.c:2:
c:/ken1/slask/straw/SP-5.32.1.1/c/lib/gcc/x86_64-w64-mingw32/8.3.0/include/avx512vnniintrin.h:25:2: error:
 #error "Never use <avx512vnniintrin.h> directly; include <immintrin.h> instead."
 #error "Never use <avx512vnniintrin.h> directly; include <immintrin.h> instead."
  ^~~~~

C:\ws\git\perl5\ext\Errno>perl Errno_pm.PL

C:\ws\git\perl5\ext\Errno>perl Errno_pm.PL
In file included from includes.c:3:
c:/ken1/slask/straw/SP-5.32.1.1/c/lib/gcc/x86_64-w64-mingw32/8.3.0/include/fmaintrin.h:25:3: error: #error "Never use <fmaintrin.h> directly; include <immintrin.h> instead."
 # error "Never use <fmaintrin.h> directly; include <immintrin.h> instead."
   ^~~~~

Unfortunately there is obviously no way to know if future headers in a mingw64 will change the assumption here, but after checking the headers it appears that it is the immintrin.h/x86intrin.h that are required to be 'first' so the code simply ensures that they come in that order (and also that both they and the rest are ordered by sorting the two categories - helps when troubleshooting so the results are always the same...).

Hope this helps,

ken1

[jkeenan]

@kenneth-olwing, our test suite has certain metadata tests (make test_porting) which your p.r. does not yet pass. Could you please apply this patch to your p.r. and git push -f to your repository? (That will enable our C.I. tests to pass on the first go-round.)

$ git diff |cat
diff --git a/AUTHORS b/AUTHORS
index bd3984f578..d49adf57b2 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -782,6 +782,7 @@ Ken Williams                   <[email protected]>
 Kenichi Ishigaki               <[email protected]>
 Kenneth Albanowski             <[email protected]>
 Kenneth Duda                   <[email protected]>
+Kenneth Olwing                 <[email protected]>
 Kent Fredric                   <[email protected]>
 Keong Lim                      <[email protected]>
 Kevin Brintnall                <[email protected]>
diff --git a/ext/Errno/Errno_pm.PL b/ext/Errno/Errno_pm.PL
index b584af0894..4130d67347 100644
--- a/ext/Errno/Errno_pm.PL
+++ b/ext/Errno/Errno_pm.PL
@@ -2,7 +2,7 @@ use ExtUtils::MakeMaker;
 use Config;
 use strict;
 
-our $VERSION = "1.36";
+our $VERSION = "1.37";
 
 my %err = ();
 

Thank you very much. Jim Keenan

[kenneth-olwing]

@jkeenan Thanks for your help.

ken1

[jkeenan]

@kenneth-olwing, please see the CI failure reported at https://github.com/Perl/perl5/runs/7672753925?check_suite_focus=true (Windows mingw64).

[tonycoz]

I wonder if it would be more robust if get_files() returned the file names in the order seen, rather than randomly per hash randomization.

The headers that depend on immintrin.h (as implemented in gcc-12 at least) don't require that they be included from immintrin.h, just that it has already been included.

[kenneth-olwing]

Hi,

Being new in this repo I took the cowardly way of fixing things as local to the win32 build as possible...:-).

Having said that I now see that my last push eff'd up a lot of whitespace for no good reason :-(. Sorry. I'll fix that, the 'real' changes are just muddied now.

Having get_files() return things in order found won't really change the underlying problem as it would still be generally non-determinate whether you'd get the files in an order that satisfies the wish of the headers to have the two files be included first. There is no externally imposed order that can anticipate this or any future similar 'rule'. I only have access to gcc 8.3.0 (strawberry perl) so I have no idea if gcc 12 have changed/added to this in any way...?

However, it's certainly a good idea to always do this in a specific order so as to ensure results are more easily repeatable - hence my change to use them in a sorted order (which incidentally will, in my circumstance actually 'fix' the problem, but that's just luck so...).

I'm not truly familiar with the script overall and would be hesitant to start mucking around in areas of the script that clearly has existed a long time and as this particular problem apparently hasn't been noticed on other platforms, it feels wrong to try to fix what ain't broken...but I'm open to suggestions.

ken1

[kenneth-olwing]

FYI, minimized the whitespace diffs for now to make the functional change be visible, but the file should be sanitized on this score; there's a mix of tab/space indentation in it...

[tonycoz]

Having get_files() return things in order found won't really change the underlying problem as it would still be generally non-determinate whether you'd get the files in an order that satisfies the wish of the headers to have the two files be included first.

For the cases where get_files() returns multiple files it gets the list of files by preprocessing a program like:

#include <errno.h>

and scanning the result for #line directives, so I don't see how it wouldn't result in a reasonable order.

[kenneth-olwing]

Doh. Mea culpa. I was so focused on the fact that the previous fix did filename processing that I assumed get_files() was finding the names in a non-determinate manner to begin with; hence I just put my fixes in the same place with a similar idea.

Ok, I'll redo on that premise. BRB

Thanks,

ken1

[kenneth-olwing]

@jkeenan @tonycoz

Would it be possible to approve the fix as it is now for the workflow actions?

FWIW, I'm using this fix when trying to get to grips with #20081 and haven't seen any adverse issues with it in that admittedly limited scope.

Thanks,

ken1

[jkeenan]

@jkeenan @tonycoz

Would it be possible to approve the fix as it is now for the workflow actions?

I hit the workflow button. Since I don't have access to, or knowledge of, Windows, others will have to examine your Windows-specific revisions.

[kenneth-olwing]

Thank you; do we need to repeat for new pushes or will it start up automatically?

[jkeenan]

Thank you; do we need to repeat for new pushes or will it start up automatically?

I suspect we will need to repeat, since for the purpose of this pull request you are a "First-time contributor." But I'm not an expert on our Continuous Integration setup, so don't take what I say as gospel truth. (@Toddr, @atoomic) (If you do IRC, this is the sort of question that can often be answered more quickly by hopping on irc.perl.org #p5p.

[kenneth-olwing]

Ok, thanks again.

[kenneth-olwing]

@xenu @atoomic @tonycoz

Would it be possible to get this PR completed?

Thanks, ken1

[toddr]

Thank you; do we need to repeat for new pushes or will it start up automatically?

I suspect we will need to repeat, since for the purpose of this pull request you are a "First-time contributor." But I'm not an expert on our Continuous Integration setup, so don't take what I say as gospel truth. (@toddr, @atoomic) (If you do IRC, this is the sort of question that can often be answered more quickly by hopping on irc.perl.org #p5p.

@kenneth-olwing A force push of the same branch should re-start CI testing.

[kenneth-olwing]

@kenneth-olwing A force push of the same branch should re-start CI testing.

Ok thanks; not sure if you're just responding to that particular point, but otherwise this is IMHO now ready to merge unless there is some other objection.

[tonycoz]

Would it be possible to get this PR completed?

The commit message on the only commit is: rebased + previous commits squashed + ensuring get_files() list has only unique entries

which doesn't describe what the PR is meant to fix.

Other than that it's good.

[kenneth-olwing]

Ah. Obviously. Fixed, again rebased and force pushed.

[kenneth-olwing]

@toddr For the record, it doesn't appear that a force push automatically restarts the workflow. I'm assuming that it's because I'm still a first-time contributor. Can any kick it off please?

[toddr]

@toddr For the record, it doesn't appear that a force push automatically restarts the workflow. I'm assuming that it's because I'm still a first-time contributor. Can any kick it off please?

I see a run 8 hours ago. https://github.com/Perl/perl5/runs/7992394167?check_suite_focus=true

Looks like the workflow doesn't trust you. github security at work.

This workflow is awaiting approval from a maintainer in https://github.com/Perl/perl5/pull/20136

I have approved it.

[toddr]

I also see a failure on porting/authors.t

A failure on this run aborts the other jobs.

https://github.com/Perl/perl5/runs/7992394173?check_suite_focus=true

[jkeenan]

I also see a failure on porting/authors.t

A failure on this run aborts the other jobs.

https://github.com/Perl/perl5/runs/7992394173?check_suite_focus=true

@kenneth-olwing, can you rebase the p.r. on blead and force push? There have been some commits in the past 24 hours which may have resolved this problem. Thanks.

[kenneth-olwing]

Rebased and ran Porting/updateAuthors.pl. Hope that brings it in sync :-)

[jkeenan]

This pull request is now passing CI on GitHub. Can we get it looked at by people knowledgeable on Windows?

[kenneth-olwing]

Thank you all for input and help!