Hi,

I would like to raise some concerns about the current implementation of ordinal bithelpers in r 43417.

I do not have the rights to re-open that issue, and I did not want to open a new one for it.

My concerns basically are the same I described in the associated bugtracker issue (https://bugs.freepascal.org/view.php?id=36146)

1.
Minor issue.
Defining the ranges TByteBitIndex will confuse users if they define a constant array with that range and then compile their program in either mode fpc or mode tp.

I have something like:
const
Expected: array[TIntegerBitIndex] of Integer = ({16 entries here if Integer is 16-bit like in mode fpc/tp});
It fails to compile with: Error: Expected another 16 array elements

(Example taken form the test program in the bugtracker)

Inside the sysutils unit the use of definition is OK, since this unit will alway be compiled in mode objfpc.

Using an integer type as the index Index parameter would "fix" that, at the cost of having runtime errors if the index was out of bounds for the ordinal used (e.g. SomeByte.SetBit(31)).

I can live with the current situation though.
My test program just redefines the ranges to properly reflect the mode the program is compiled in.

2.
A rather more serious issue.
Compile time errors occur with e.g.
ANativeInt.SetBit(High(TNativeIntBitIndex)) in modes tp (32-bit), fpc (32-bit), objfpc (32+64-bit) and delphi (32+64-bit) Range check error while evaluating constants (2147483648 must be between -2147483648 and 2147483647)
Same error for AnInteger.SetBit(High(TIntegerBitIndex)) in modes objfpc (32-bit), delphi (32-bit) and macpas (32-bit)

The (tested) remedy for this is to do all bit manipulations on the unsigned ordinal instead.
This will get rid of all compile time errors.
It will then also pass all tests in the test program for all modes for both 32 and 64 bit.
(I can't test 16-bit compiler).

@Michael: I missed your request to me to make a new patch after I fixed the missing parnethesis in Thaddy's latest ClearBit method, otherwise I would have posted that for in the bugtracker.

Yes, I had the same error when recompiling the test program with the local indexes removed.

Inside the sysutils unit the use of definition is OK, since this unit will alway be compiled in mode objfpc.

That is why I decided to keep it: the mode of sysutils is known and will never change. A user is supposed to take this into account.

I am aware of your remark. That is why I defined TLongIntBitIndex. I decided to keep the TIntegerBitIndex name because the helper is for the integer type, not for the longint type.

This error was confirmed as a compiler bug. It also disappears if you remove the inline from the methods.

Since there is no run-time error for the same value, I kept the code as-is. I consider this a remote corner case.

If you send me a patch to rework the bit manipulations, I will check & apply it.

Michael.

AM Michael Van Canneyt wrote

That is why I decided to keep it: the mode of sysutils is known and will never change. A user is supposed to take this into account.

OK.

[snip]

If you send me a patch to rework the bit manipulations, I will check & apply it.

If this is considered a compiler bug, then my patch (which would consist of typecating to unsigned) is a workaround tha simply hides the compiler error.
So, it should not be applied then, as you already decided,

Thank you for explaining.

B.t.w. I really enjoyed doing the extensive testing. Apart from the cornercase compiler-time issues, all test are ok for all modes on 32 and 64 bit.
So, I'm happy.

Bart

When testing I converted the tests to the format suitable for my rtl testsuite, so they are run whenever I run that testsuite.

Michael.

Michael Van Canneyt wrote

When testing I converted the tests to the format suitable for my rtl testsuite, so they are run whenever I run that testsuite.

They're even more useful if they can be run by anyone, and are automatically used during the nightly testsuite run.

Jonas

I pushed the suite in svn, rtl/test.

They're completely standalone, depend on system unit only, the testsuite functionality is in unit ptest. (there are tests for that too).

Let me know what you need extra to be able to run them with the compiler testsuite.

Normally you can run them and have them report an exit code as the usual testsuite program. With minor changes they can probably produce a report similar to the one used by the compiler testsuite: the output is configurable just as in fcl-fpcunit.

Michael.

Michael Van Canneyt wrote

Let me know what you need extra to be able to run them with the compiler testsuite.

Do you know which tests were copied from the existing testsuite at some point? E.g. utrwsync.pp is derived from an old (buggy) version of tests/test/sysutils/trwsync.pp

Normally you can run them and have them report an exit code as the usual testsuite program. With minor changes they can probably produce a report similar to the one used by the compiler testsuite: the output is configurable just as in fcl-fpcunit.

We don't need custom-formatted reports for the testsuite, just exit codes from individual test programs. It seems we can easily create those by simply creating programs that include individual units. The punit unit can be moved to tests/tstunits when the other ones are moved to the appropriate directories under tests/test/units, so it will be automatically found when compiling them.

Jonas

No, I don't know that. Where possible I kept the name, but not always. if a test was changed after I converted it, I have no way of knowing.

No moving, please.

We discussed this before: I want the tests to reside with the units that they test. You can copy all you want, but I will only work with the tests in the rtl directory and have no intention of maintaining a copy of the tests in the testsuite. If some old test fails in the compiler testsuite, I will convert it to my suite. The above is one I seem to have missed.

I have extensively argued before why I think the testsuite is completely unsuitable for testing single unit functionality, I will not repeat my arguments again.

I will give all help you need in order to integrate that suite somehow with the daily test run, but that help is subject to the above condition.

If you think this is unacceptable, no problem, I will remove my testsuite code again from svn, and I will keep using it in private as I did till now.

Michael.

It just clutters the rtl dir without any general use.

But then you have to maintain it in a way that it is runable on all targets by adding proper makefiles etc., the docompile.sh is just useless as it tests only with the installed fpc not that one from the tree, there is no way to cross test, remote test, emulator test, pass compiler options etc.

I have extensively argued before why I think the testsuite is completely unsuitable for testing single unit functionality, I will not repeat my arguments again.

Well, I still call them void :)

I will give all help you need in order to integrate that suite somehow with the daily test run, but that help is subject to the above condition.

Michael Van Canneyt wrote

I have extensively argued before why I think the testsuite is completely unsuitable for testing single unit functionality, I will not repeat my arguments again.

We can add testsuite makefile targets that only run the tests under units, or even only for a specific directory under units.

Jonas

The same directories exist in many of the packages, with fcl-fpcunit based tests. I don't hear anyone complaining. What matters is we have the tests.

It is to make this all possible that I offered to help.

I am aware that the docompile.sh is suitable for my setup only;
I didn't claim this was the solution for the testsuite.

I suspect that for integration in the testsuite, one or more separate programs must be made. Most likely some copying will need to be done. All no problem for me.

As I said: I have only 1 condition: the sources stay in the rtl dir so test results can be obtained in a matter of seconds, not minutes.

I have no doubts on that score. The discussion ended without agreement. That is why I didn't bother to add my code to SVN.

But I (must) do the tests, and I intend to do them in a way that suits my style of development.

But if you prefer, I can remove the code again, no problem. it's your call.

Michael.

Running the testsuite (even part of it) requires too much preparation and takes too much time. It's also badly organized, you cannot easily run a single test if you need that.

The compiler testsuite is suitable for the compiler. I don't argue there, it takes waiting while the compiler is invoked on all test programs, this is unavoidable. To make sure some change is not breaking something you must run all tests. You are used to it, so you see nothing wrong with it. That's OK.

But this way of working is way too slow & cumbersome for me.

I definitely want to help to integrate the tests somehow in the daily testrun, but I will not use the slow testsuite.

Michael.

Yes. As the rtl is fundamental, it is tested together with the compiler.

What matters is we have the tests.

Yes. But I see no point in having the rtl tests cluttered to different locations.

It is to make this all possible that I offered to help. I am aware that the docompile.sh is suitable for my setup only;

See? This is what I do not like about this approach :) It is for your setup only.

I see no point in such duplication of code which must be manually updated.

As I said: I have only 1 condition: the sources stay in the rtl dir so test results can be obtained in a matter of seconds, not minutes.

But you expect the compiler developers to do so? Changes in the compiler affect often the rtl. So when testing the compiler, it is useful to test the rtl as well. As the compiler is tested by tests/ rtl tests are located there as well.

If everybody starts to add it's own way of testing, we will end up with a big mess.

But if you prefer, I can remove the code again, no problem. it's your call.

Michael Van Canneyt wrote

Running the testsuite (even part of it) requires too much preparation and takes too much time.

For testing RTL units, all that would be required is a) compile the RTL
b) cd tests
c) make TEST_FPC=fpc tests_all -j 4 (or e.g. make TEST_FPC=fpc tests_rtl_sysutils -j 4)

It's also badly organized, you cannot easily run a single test if you need that.

Why not?

The compiler testsuite is suitable for the compiler.

It's not just a compiler testsuite. It's a complete testsuite, both for the compiler and for units.

That's why I proposed to add support for running only the tests specific to the RTL and/or specific units.

I definitely want to help to integrate the tests somehow in the daily testrun, but I will not use the slow testsuite.

With parallel compilation, it will be barely slower.

Jonas

~/fpc/git/fpctrunk$ fpc tests/test/units/system/tval -v- Free Pascal Compiler version 3.0.4+dfsg-18ubuntu2 [2018/08/29] for x86_64 Copyright (c) 1993-2017 by Florian Klaempfl and others Target OS: Linux for x86-64
Compiling tests/test/units/system/tval.pp
Linking tests/test/units/system/tval
/usr/bin/ld.bfd: warning: link.res contains output sections; did you forget -T?
31 lines compiled, 0.1 sec
~/fpc/git/fpctrunk$ tests/test/units/system/tval
Test val for longint type
All tests succeeded count=259958
Test val for dword type
All tests succeeded count=519916
Test val for int64 type
All tests succeeded count=779874
Test val for qword type
All tests succeeded count=1039832

?

And you don't call this unix only docompile.sh not cumbersome with the compiler parameters in some configuration file?

I definitely want to help to integrate the tests somehow in the daily testrun, but I will not use the slow testsuite.

As said, this is one of the void arguments, on a modern CPU it takes <1 min.

Op 2019-11-09 om 18:28 schreef Jonas Maebe:

That's why I proposed to add support for running only the tests specific to the RTL and/or specific units.

A tagging system for tests in the .pp tests might be the best? Occasionally some central admin would have to be updated (automatically) and committed. This could even be a byproduct of some nightly run. Then just run enable the drive to run the testsuite for certain tags? (RTL or a specific unit).

This means that tests adding won't really change, but partial runs become possible.

Op 2019-11-09 om 18:29 schreef Florian Klämpfl:

I hope that is not a unix only figure :-)

Florian Klämpfl wrote

> What matters is we have the tests. Yes. But I see no point in having the rtl tests cluttered to different locations.

Exactly.

My solution is WAY superior to yours in this respect.

I have exactly 1 application which I open in lazarus. I can navigate between all the 100ds of tests without problem.

Compile, run, ready. It's a matter of seconds.

You, on the other hand, have 100-ds of applications which cannot be navigated or opened in lazarus without a dialog asking for a program to be created.

Let alone try finding where a particular function is tested from inside the IDE.

Well, I don't intend to argue any more, I consider it utterly pointless. We clearly will not meet each other, I already understood this last time.

So, I have removed the code again from SVN, so we're back to where we were yesterday.

Michael.

Yes, if you ignore that you need to handle different compilers/targets (installed/trunk/fixes/cross compilers/remote testing).

Jonas Maebe wrote

> I definitely want to help to integrate the tests somehow in the daily > testrun, but I will not use the slow testsuite. With parallel compilation, it will be barely slower.

home:~/rtl> time retest Classes > out
3.400u 0.628s 0:04.02 100.0% 0+0k 0+33872io 0pf+0w

retest is the command I use. I redirected output. 3 seconds, and that includes recompiling the RTL. All classes tests run.

home:~/fpc> time make clean all PP=ppcx64-3.0.4 > out /usr/bin/ld: warning: x86_64/bin/x86_64-linux/link.res contains output sections; did you forget -T? 119.139u 11.763s 1:47.32 121.9% 0+0k 179800+1292024io 2pf+0w

home:~/fpc> time make clean rtl packages > out
61.289u 4.929s 0:31.64 209.2% 0+0k 0+728024io 0pf+0w home:~/fpc>

with -J7 and not even testuite run.

Michael.

I would ignore this in each case. I compile & test for the platform I work on.

The rest would be for the daily testsuite run. That is why it exists. Or do you intend to say you manually run the tests for all platforms after every line you change ? I think we would not see many improvements if you did.

I am aware of the fact that I run only 1 platform. It is also the reason I offered to help integrating it in the testsuite, so the nightly testsuite can pick it up for all platforms.

But I will not change my way of working.

1 minute and more or 6 seconds is a world of difference, and a showstopper for me: I tried your way, it didn't work for me.

I have demonstrated it amply I think, so if you don't accept that, OK, a pity.

The offer to help integrating the local tests somehow in the testsuite remains.

Michael.

This is way too late. This would result in a continuously broken compiler/rtl.

Or do you intend to say you manually run the tests for all platforms after every line you change ?

Almost ;), but 2-3 platforms and those change obviously as I test a lot just with qemu. But indeed, before every commit I test on i386 and x86-64 (linux or win) and additionally on the target the patch was developed, e.g. arm or e.g. on an architecture which uses a different endianess or requires proper alignment. If the patch is architecture specific, I test the affected architectures. I had for sure tested bit fiddling code like the bit helpers on a big endian target.

Florian Klämpfl schrieb am Sa., 9. Nov. 2019,
20:45:

But if they're used for the daily run they'd also be used for the ones run manually? I mean I did of after all for the Rtti unit tests as well...

Regards,
Sven

As I mentioned, we could change it so that for running the hypothetical tests_rtl target in tests directory, you would only have to compile the RTL.

Jonas

BTW, I don't see a -j 7 in the above commands. You should also add FPMAKEOPT="-T 7" to parallises the compilation of the packages and utils directories.

Jonas

I broke my brains and could not conclude anything else than a compiler bug in these cases: setting the sign bit is all I do and the bit patterns are all correct.
It seems that this is also confirmed. Anyway you suggestion to do the internals only as unsigned types makes sense if in that case the sign bit is respected.
Funny how such simple code - which it is - can have so much complexity.

I have environment variables set so I don't need to specify that.

Michael.

No.

It's not just the waste of time, it's also the origanization of the sources. See my mail to Florian. I have 1 application that has all the tests. It's easy to navigate & whatnot.
In the testsuite the tests are scattered over 100-ds of applications in which a cat cannot find her kitten. I wouldn't know how to find all tests for a particular function apart from a grep X *.* */*.* */*/*.* etc. Not particularly efficient.

You guys run & debug the compiler. I run & debug the test app. This is a fundamental difference in work cycle.

The compiler way is no way of working for me.
I understand it suits you: fine, I don't stop you.

But please, never mind.

It's clear we won't find each other. I have removed the sources from SVN since Florian didn't want the rtl cluttered and we'll leave .

The nice thing about your punit-based tests is that they can just as easily be used in a single test application as in multiple individual test applications. Since the testsuite only tries to compile and execute source files starting with "t", there would be no problem with also adding your single top-level application there as long as its name starts with another character.

We could even have just the single top-level application in the testsuite (along with all of the individual subject tests as units, like it was in the code you added to svn), since for testing unit functionality having separate test programs is indeed less important (and it's still very easy to make one if it is necessary for some reason at one point).

It's clear we won't find each other. I have removed the sources from SVN since Florian didn't want the rtl cluttered and we'll leave it at that.

The problems with this are
1) your tests never get run on all platforms, so things may be broken on platforms other than the ones you test
2) when someone else fixes a bug in or improves one of the tested routines, they may break on of your tests without realising (and without being able to easily test their changes, unless they write a test program similar to one you may already have and then add that to the testsuite themselves)

Jonas

All tests written for a certain unit written as "unit unit tests" are in tests/test/units/<unit name> (actually, tests/test contains in general unit tests - unit in the sense of testing - all other dirs are regression tests). Your test app could easily live there e.g. in tests/test/rtl if it is not split between different units. All test runs (nightly or manual) would pick it up.

The funny part of this discussion is: if we add all system unit tests into Michael's scheme and his single test application, running this single test would probably more time than running currently all regression tests. theapthread testing heap management and threading takes alone 35 sec :)