Untested fix for PRP optional how_far_factored,tests_saved

[Artoria2e5]

Is it alright if I just let the CI do it?

Should fix #30, gotta figure out if there's aplace to add test cases though!

[tdulcet]

Thanks for your PR!

I enabled the CI. It looks like there are some compiler errors that would need to be resolved.

Note that P-1 Pminus1= assignments also follow a similar format, with optional parameters, followed by known factors at the end, so you may want to test that and potenchally reuse the parsing code for both:

Pminus1=k,b,n,c,B1,B2[,how_far_factored][,B2_start][,"factors"]

gotta figure out if there's aplace to add test cases though!

Unfortunately, there are currently no tests for the assignments parsing. The CI just runs the Mlucas self-test mode with various combinations of options, which mainly tests the FFT code. Your changes would need to be manually tested.

[Artoria2e5]

Two problems. One is bad integration of my code. Other is no memrchr on mac. Can fix the first rn

[Artoria2e5]

For the latter I think https://lists.zx2c4.com/pipermail/cgit/2020-August/004509.html looks good. Gotta sleep!

[Artoria2e5]

Huh. Might as well try the agent mode since I still want to catch more sleep. Actually forget about going back to sleep (that almost never worked! worse chances than writing code without tests) I've gotta change the makemake

(subcontracting it out at: Artoria2e5#1)

[Artoria2e5]

Someone is gonna need to fix the 1,048,576 or however many clang-tidy errors sometime. Not here though!

[Artoria2e5]

I want to do something to the workflow thing so that a fast, perhaps one-compiler-per-OS, no-analyzer "smoke test" can be automatically done on each push without approval, but. Welp, I can run the test from my own repo, so get the test state there: https://github.com/Artoria2e5/Mlucas/actions/runs/19097904155

Also, the current main baseline seems to have left something dubious in the form of an undefined twopmodq100_2WORD_DOUBLE_q4, which Windows clang complains about. In fact it is weird that the other platforms don't complain about it. Also weird is twopmodq100_2WORD_DOUBLE(, which is defined nowhere but used just fine??

Another complaint, from Windows clang, that I have nothing to do with is:

../src/radix144_main_carry_loop.h:144:29: error: assignment to 'vec_dbl *' {aka 'struct double_x4 *'} from incompatible pointer type 'vec_dbl **' {aka 'struct double_x4 **'} [-Wincompatible-pointer-types]
  144 |                         tm1 = rad9_iptr;        // Stash head-of-array-ptrs in tmps to workaround GCC's "not directly addressable" macro arglist stupidity
      |                             ^
../src/radix144_main_carry_loop.h:145:29: error: assignment to 'vec_dbl *' {aka 'struct double_x4 *'} from incompatible pointer type 'vec_dbl **' {aka 'struct double_x4 **'} [-Wincompatible-pointer-types]
  145 |                         tm2 = rad9_optr;
      |                             ^
../src/radix144_main_carry_loop.h:634:29: error: assignment to 'vec_dbl *' {aka 'struct double_x4 *'} from incompatible pointer type 'vec_dbl **' {aka 'struct double_x4 **'} [-Wincompatible-pointer-types]
  634 |                         tm0 = rad9_iptr;        // Can't use tm1 here since use that for s1p00 offsets in loop body
      |                             ^
../src/radix144_main_carry_loop.h:635:29: error: assignment to 'vec_dbl *' {aka 'struct double_x4 *'} from incompatible pointer type 'vec_dbl **' {aka 'struct double_x4 **'} [-Wincompatible-pointer-types]
  635 |                         tm2 = rad9_optr;        // Stash head-of-array-ptrs in tmps to workaround GCC's "not directly addressable" macro arglist stupidity

From a type soundness perspective of course it's right, but we're really micromanaging the register use here. Probably just put a (void *) after the = to get rid of the error.

In the meantime windows 11 arm gcc just has no gmp. Very funny.

[Artoria2e5]

Yeah, of #^!@ course the arm gcc has no gmp. CI file says:

    - name: Install
      if: ${{ ! endsWith(matrix.os, '-arm') }}
      run: |
        pacman -S --noconfirm "${env:PACKAGE_PREFIX}gmp" "${env:PACKAGE_PREFIX}hwloc"
        & $env:CC --version

And the packages we want clearly exist, so I'm not even sure why it's skipped:

• https://packages.msys2.org/packages/mingw-w64-clang-aarch64-gmp
• https://packages.msys2.org/packages/mingw-w64-clang-aarch64-hwloc

[Artoria2e5]

I think I will give up on making AVX512 build on Windows. Mode should really be another CI Matrix choice if you think about it...

Yikes! Windows-arm is using mingw64 instead of the complete msys!

[tdulcet]

Someone is gonna need to fix the 1,048,576 or however many clang-tidy errors sometime.

Many of those errors should already be fixed in #34. We just need help reviewing that massive PR.

Also, the current main baseline seems to have left something dubious in the form of an undefined twopmodq100_2WORD_DOUBLE_q4

This is a longstanding known issue with Mfactor (build from the same Mlucas codebase), which Ernst was aware of. Many of the Mfactor build combinations currently fail to compile on all OSs.

In the meantime windows 11 arm gcc just has no gmp. Very funny.

Yes, see #45 (comment) for the reason. The Windows builds are largely broken, but that should be fixed by #45. As long as your changes successfully compile on Windows without additional warnings, feel free to ignore the runtime errors.

[Artoria2e5]

In theory my change should only affect… well, one object file, so honestly I have zero idea how something that used to build suddenly isn't building. Perhaps I should roll back all the CI attempts and just try to rerun the upstream main first. Then I have a baseline to compare against, and possibly a case for arguing that I'm not causing additional failures.

[Artoria2e5]

I can confirm that the Windows build failures have nothing to do with me. https://github.com/Artoria2e5/Mlucas/actions/runs/19122012046/job/54644284595 Rolling back to just the minimum commits and going to flag ready again.

Oh wait, I could see that in https://github.com/primesearch/Mlucas/actions/runs/19001848411/job/54269671000. What was I doing??

Anyways, up to whatever point in https://github.com/Artoria2e5/Mlucas/actions/runs/19097904155/job/54562362973, it builds.

[Artoria2e5]

Might as well also incorporate the PM1 thing you mentioned. Also test on my actions before yours. Draft.

---

Waiting on https://github.com/Artoria2e5/Mlucas/actions/runs/19129486171

[Artoria2e5]

It's going well so far. I am going to mark "ready" again. In the meantime I should open another branch to get Windows CI sorted.

[tdulcet]

I can confirm that the Windows build failures have nothing to do with me.

Thanks for looking into it. I have the CI scheduled to run once a month, to catch issues like this. It looks like the Windows CI broke some time in September (it was successful on September 1, but failed October 1), most likely because GitHub upgraded the version of GCC, which turned one of the previous warnings into an error. This should be fixed by #34, which resolves all of the remaining compiler warnings. We may need to extract the relevant commit out from #34 if this needs to be fixed sooner.

Regarding your PR, this looks like a lot of added complexity for a single use of memrchr. Could we possibly refactor this to use a different function instead?

[Artoria2e5]

Memrchr is about as simple as it goes: scan the memory region right to left. GNU has it added to their libc because it's a pretty common use case.

The complexity comes in because we try to detect whether the system has it. If we just called it mymemrchr() and used it always without caring about the system, we'd save the apparent complexity. If you want it that way, I can do it. There's not much performance to care about in scanning a typically < 80-byte region anyways.

As a side note, I think the current code is using too much strstr() for single-characters when it should be strchr(). Will it be okay if I replace them? There's no real performance impact (again, lines are short!), only "feels right" impact.

[Artoria2e5]

Of course, when it comes to detecting whether GNU-extension funcs are available and taking steps to replace them -- as well as writing the most contrived script to work around ancient Bourne shell bugs -- very few things can beat GNU's own solution: gnulib + autotools. It might even end up easier for whoever wants to build the project just because the process is more familiar and standardized.

It's definitely not less complex for us though. Assuming bash is present makes shell scripts much easier to write. Worst case scenario, the user needs to build bash first.

Huh, this does open up another topic: perhaps we should have a configure script that somewhat resembles the GNU stuff? Perhaps we should encourage re-running make outside of makemake? Have some make install even?

---

I might be hypomaniac. I should set a delay on everything I send.

[tdulcet]

If we just called it mymemrchr() and used it always without caring about the system, we'd save the apparent complexity. If you want it that way, I can do it.

Yes, that greatly simplifies it. Thanks for making this change.

As a side note, I think the current code is using too much strstr() for single-characters when it should be strchr(). Will it be okay if I replace them?

Yes, that would be fine. I have made a few similar improvements myself.

Perhaps we should encourage re-running make outside of makemake? Have some make install even?

Yes, one can run make directly, if they want to keep the same options. It is for example recommended to run make clean to remove the object files. Feel free to add additional targets. Long term, I would like to move the logic from the makemake.sh script into a Makefile, which would simplify the build process and remove the dependency on Bash. The config-fermat.sh script should be integrated directly into Mlucas, similar to the existing Mersenne self-tests.

[tdulcet]

We will need to keep some of this code that sets the test type and PRP base. We should also keep the assert statements for the PRP base and residue type, as well as the relevant comments.

[Artoria2e5]

I think I only meant to remove the extract_known_factors part. Oops, will fix.

[tdulcet]

This looks good to me. We will just need to manually test it with all variations of the optional values for both the PRP=/PRPDC= and Pminus1= assignment formats.

[tdulcet]

Since we are now parsing assignments from both ends, should we not add a check that cptr == startq, to confirm that there were no unexpected values in the middle.

[Artoria2e5]

Will do.

I am starting to feel that a rewrite some time in the future that starts parsing with a "tokenizing" step would be beneficial. As in, split the incoming cstr into tokens represented by two arrays:

// well, we already have a bunch of global variables in the parsing part..
#define MAXTOK 32
char *(tokens[MAXTOK]); /* forgot how operator precedence worked here, ah just parenthesize */
char tokflags[MAXTOK]; /* is it quoted? not that necessary for a "liberal" parse */
int tokenize(const char* s); /* returns token count (nonnegative) or error (negative) */

... then the rest of the code basically operates on spans of [token[i], token[i+1]), until token[i+1] is NULL. Don't want to touch more than what's necessary now though.

[the abstract syntax the tokenizer layer would deal with is:

Line = Tag '=' TokenList
TokenList = '' | Token | Token ',' TokenList
Token = QToken | NQToken
NQToken = REGEX([^",]+)
QToken = REGEX([^"]+)

The quotes being elevated to not just the end because if I recall correctly there are additional Prime95/mprime-specific uses of quoted strings in worktodo.
]

[tdulcet]

Suggested change

	int found = 0;
	bool found = FALSE;

[Artoria2e5]

We have stdbool? Great.

[tdulcet]

Yes, Ernst had a custom boolean type, but I changed it to use stdbool.h in #22, since this is part of C99.