Handle intrin files on win32 with gcc #20033

kenneth-olwing · 2022-08-03T12:42:40Z

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 · 2022-08-03T20:03:46Z

@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                   <ken@mathforum.org>
 Kenichi Ishigaki               <ishigaki@cpan.org>
 Kenneth Albanowski             <kjahds@kjahds.com>
 Kenneth Duda                   <kjd@cisco.com>
+Kenneth Olwing                 <knth@cpan.org>
 Kent Fredric                   <kentfredric@gmail.com>
 Keong Lim                      <Keong.Lim@sr.com.au>
 Kevin Brintnall                <kbrint@rufus.net>
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 · 2022-08-04T07:52:56Z

@jkeenan Thanks for your help.

ken1

jkeenan · 2022-08-04T13:52:47Z

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

tonycoz · 2022-08-08T02:06:24Z

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 · 2022-08-08T08:48:51Z

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 · 2022-08-08T14:11:10Z

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 · 2022-08-09T01:09:53Z

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 · 2022-08-09T06:10:34Z

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 · 2022-08-15T18:59:03Z

@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 · 2022-08-15T21:08:42Z

@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 · 2022-08-15T21:28:05Z

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

jkeenan · 2022-08-15T22:12:29Z

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 · 2022-08-15T22:16:21Z

Ok, thanks again.

tonycoz · 2022-08-16T00:30:37Z

ext/Errno/Errno_pm.PL

 		if (/$pat/o) {
 		   my $f = $1;
 		   $f =~ s,\\\\,/,g;
-		   $file{$f} = 1;
+		   push(@file, $f);
 		}
 	    }
 	    else {
-		$file{$1} = 1 if /$pat/o;
+		push(@file, $1) if /$pat/o;
 	    }
 	}
 	close(CPPO);
    }
-    return keys %file;
+    return @file;


This adds duplicate entries to get_files() which really isn't necessary.

Rather than using an array I was thinking of something more like:

my $seq = 1; while(<CPPO>) { if ($^O eq 'os2' or $IsMSWin32) { if (/$pat/o) { my $f = $1; $f =~ s,\\\\,/,g; $file{$f} = $seq++ if !$file{$f}; } } else { $file{$1} = $seq++ if /$pat/o && !$file{$f}; } } close(CPPO); } return sort { $file{$a} <=> $file{$b} } keys %file;

This returns the filenames in the order seen and avoids returning duplicate entries.

Yes, I was aware, and initially tried to use List::Util::uniq on the return; alas, this is not functioning at that point (because it being a miniperl IIUC). So I figured it didn't truly matter, including the same file many times shouldn't cause a problem.

But yes, I checked and there's a fair amount of unnecessary duplication, so I made a local uniq() function to be used for the return list. I think this a bit clearer than having the %seen hash in the main part, and since it's always a list, it will also work for any of the other OS/variant cases in case they have more than one entry in the array.

Pushed a rebased/squashed commit with all changes (also getting rid of some commits I made from work which uses a different mail address in the commit, causing the porting/author.t to complain).

?

kenneth-olwing · 2022-08-22T20:26:29Z

@xenu @atoomic @tonycoz

Would it be possible to get this PR completed?

Thanks,
ken1

toddr · 2022-08-22T20:54:13Z

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 · 2022-08-22T20:59:32Z

@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 · 2022-08-24T01:38:39Z

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 · 2022-08-24T06:13:32Z

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

kenneth-olwing · 2022-08-24T07:07:59Z

@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 · 2022-08-24T14:29:04Z

@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 · 2022-08-24T14:30:13Z

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 · 2022-08-24T14:37:01Z

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.

This fixes #20033. When building on Windows with Strawberry 5.32.1 (gcc 8.3.0) as the toolchain, the Errno.pm is created by a script Errno_pm.pl, which takes output from the compiler to find headers. A subset of these headers requires them to only be included by some specific headers. Previously the header order was effectively random and this occasionally caused build errors (that further were never detected). The get_files() is now returning the header names in the order the compiler saw them which insures they are in the right order.

kenneth-olwing · 2022-08-24T14:57:10Z

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

jkeenan · 2022-08-24T18:41:49Z

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

kenneth-olwing · 2022-08-25T06:24:22Z

Thank you all for input and help!

This fixes Perl#20033. When building on Windows with Strawberry 5.32.1 (gcc 8.3.0) as the toolchain, the Errno.pm is created by a script Errno_pm.pl, which takes output from the compiler to find headers. A subset of these headers requires them to only be included by some specific headers. Previously the header order was effectively random and this occasionally caused build errors (that further were never detected). The get_files() is now returning the header names in the order the compiler saw them which insures they are in the right order.

kenneth-olwing mentioned this pull request Aug 3, 2022

Strawberry Perl POC and Status of 5.34.1 StrawberryPerl/Perl-Dist-Strawberry#39

Closed

jkeenan requested review from atoomic, tonycoz and xenu August 3, 2022 19:26

jkeenan added the distro-mswin32 label Aug 10, 2022

kenneth-olwing mentioned this pull request Aug 15, 2022

Windows builds built with gcc -O2 works on Win10 but fails on Win11 #20081

Closed

tonycoz reviewed Aug 16, 2022

View reviewed changes

tonycoz merged commit 697eaf8 into Perl:blead Aug 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle intrin files on win32 with gcc #20033

Handle intrin files on win32 with gcc #20033

kenneth-olwing commented Aug 3, 2022

jkeenan commented Aug 3, 2022

kenneth-olwing commented Aug 4, 2022

jkeenan commented Aug 4, 2022

tonycoz commented Aug 8, 2022

kenneth-olwing commented Aug 8, 2022

kenneth-olwing commented Aug 8, 2022

tonycoz commented Aug 9, 2022

kenneth-olwing commented Aug 9, 2022

kenneth-olwing commented Aug 15, 2022

jkeenan commented Aug 15, 2022

kenneth-olwing commented Aug 15, 2022

jkeenan commented Aug 15, 2022

kenneth-olwing commented Aug 15, 2022

tonycoz Aug 16, 2022

kenneth-olwing Aug 16, 2022

kenneth-olwing commented Aug 22, 2022

toddr commented Aug 22, 2022

kenneth-olwing commented Aug 22, 2022

tonycoz commented Aug 24, 2022

kenneth-olwing commented Aug 24, 2022

kenneth-olwing commented Aug 24, 2022

toddr commented Aug 24, 2022

toddr commented Aug 24, 2022

jkeenan commented Aug 24, 2022

kenneth-olwing commented Aug 24, 2022

jkeenan commented Aug 24, 2022

kenneth-olwing commented Aug 25, 2022

Handle intrin files on win32 with gcc #20033

Handle intrin files on win32 with gcc #20033

Conversation

kenneth-olwing commented Aug 3, 2022

jkeenan commented Aug 3, 2022

kenneth-olwing commented Aug 4, 2022

jkeenan commented Aug 4, 2022

tonycoz commented Aug 8, 2022

kenneth-olwing commented Aug 8, 2022

kenneth-olwing commented Aug 8, 2022

tonycoz commented Aug 9, 2022

kenneth-olwing commented Aug 9, 2022

kenneth-olwing commented Aug 15, 2022

jkeenan commented Aug 15, 2022

kenneth-olwing commented Aug 15, 2022

jkeenan commented Aug 15, 2022

kenneth-olwing commented Aug 15, 2022

tonycoz Aug 16, 2022

Choose a reason for hiding this comment

kenneth-olwing Aug 16, 2022

Choose a reason for hiding this comment

kenneth-olwing commented Aug 22, 2022

toddr commented Aug 22, 2022

kenneth-olwing commented Aug 22, 2022

tonycoz commented Aug 24, 2022

kenneth-olwing commented Aug 24, 2022

kenneth-olwing commented Aug 24, 2022

toddr commented Aug 24, 2022

toddr commented Aug 24, 2022

jkeenan commented Aug 24, 2022

kenneth-olwing commented Aug 24, 2022

jkeenan commented Aug 24, 2022

kenneth-olwing commented Aug 25, 2022