#303 pic14 - many patches - pass regression tests

[salo]

I've been working for quite a long time on the pic14 backend and finally I've got all the regression tests to pass.

The attached file contains the patches and some README files with notes about them. There are also patches to fix some issues in gputils.

Here is the summary, extracted from the README file:

---

SUMMARY

---

This bundle includes a set of patches for sdcc 3.9.0 that
provide the following enhancements to the pic14 port:

• the pic14 device lib has been rewritten to use, as much as
  possible, the generic source files that other ports use.

• a standard C library has been added

• up to 8 flavours of the libraries and the regression tests
  can be built, as a combination of:

• regular versus enhanced cores (e suffix)

• optimizations enabled or disabled (o suffix)

• experimental code disabled or enabled (x suffix)

• fixes to the src/regression tests so now all tests pass

• fixes to the support/regression tests so now all tests pass

• fixes and enhancements to the pic14 backend include:

• miscellaneous fixes needed to pass the regression tests

• implementation of character strings in initializations (wchar support)
• implementation of multi-byte bitfields
• rewrite of glue.c to be as close as possible to SDCCglue.c

• an exhaustive debug system

• and the experimental code includes:

• calls with arguments to function pointers

• variadic functions
• enhanced register allocation system

The debug modes and experimental code are disabled by default and
can be enabled at run-time with command line options.

All this is split into several patches so they can be checked
one by one.

Some required patches to gputils 1.5.0 are also provided.

[Philipp Klaus Krause]

Patch sdcc_generic_fixes:

• In mbstowcs.c b is changed from int to size_t. However, this breaks error handling for encoding errors.
• In bitfields.c, test2291335) is renamed to __test2291335, but AFAIK, this means that the function is no longer recognized as a test case by the regression testing framework.
• In gcc-torture-execute-loop-13.c, additional assertions are added. In general, we try to not add stuff to the gcc-* tests, to keep the diff between the GCC and SDCC versions of these tests small.

Excluding those 3 files, I applied the patch in [r11257].

[salo]

Thank you, Philipp. Some comments about it:

• mbstowcs.c

You are right, b should be kept of type int.
But the rest of the changes in this file fix the code to handle the case where pwcs is NULL as specified by mbstowcs(3).
I think they should be applied.

• bitfields.c

Underscores make the test to be ignored, yes.
This is the current state of the test (it is now disabled).
The patch proposes to remove the underscores to re-enable the test.

I did it just to check it, and it was useful because this test makes some checks not done by any other one. It checks initialization of structs with unnamed fields, both with zero and non-zero length, and both with global and local storage, in data space.

All the ports, except host, pass this test, so I decided to leave it enabled by default and disabled only for PORT_HOST.

• gcc-torture-execute-loop-13.c

Ok. Note that nullstring.c is in a similar situation.

[Philipp Klaus Krause]

• Looking at the C17 standard, section 7.22.8.1, it seems to me that passing a null pointer as pwcs is not allowed, and thus undefined beahviour that we don't need to handle.
• nullstring.c doesn't come from GCC, so there is no diff to keep small.

[Philipp Klaus Krause]

Patch pic14_define_enhanced:

Applied in [r11258].

[salo]

Thanks, Philipp.

Just for the record: __SDCC_PIC14_ENHANCED is (re-)defined in <pic16fam.h></pic16fam.h>

This is not needed anymore and probably should be removed: sdcc's knowledge of the 'enhanced' attribute of the core should be authoritative, as this is what decides whether enhanced instructions are used or not.

I keep in mind a lot of issues, like this one, pending to be handled. Most of them are documented in the README files. But I think they are not a priority right now. At some point in the future they could be entered in the system as bug fixes or feature requests, so they can be handled one by one.

[Philipp Klaus Krause]

If the header is changed, it should probably be done by changing support/scripts/pic16fam-h-gen.pl and regenerating the header.

[Philipp Klaus Krause]

Patch pic14_device_lib:

Applied in [r11259].

However, it would be good to go even further, mostly to avoid code duplication:

• Many header files already contain some port-specific #ifdef anyway, so pic14-specific standard library headers from device/include/pic14 could be merged into the generic ones in device/include
• For mbrtoc16 and mbrtowc, the change made for pic14 won't harm other backends, so it could be unconditionally done in the generic files.
• For bsearch.c and qsort.c, the pic14-specific stuff is small and contained enough that the #ifdef wouldn't hurt much in the generic files.
• For aligned_alloc, I don't know why we need a pic14-specific version. Currently no backends hard-require alignment, so the aligned_alloc.c always exists only to provide the possibility of getting a fuction pointer.

[salo]

With the exception of _ftoa.c and the files related to malloc.h and stdio.h, that are quite different from the generic ones, all the other files (both headers.h and sources.c) could be integrated with the generic ones.

In fact, the customized versions provided by the patch are placed in the pic14 directories to keep them apart from the generic ones, but I think that all of them, as they are currently written, are ready to replace the generic ones. (WARNING: it should be checked, and in any case this is true only with respect to sdcc 3.9.0, not any later revision).

Anyway, I think this is not a priority right now.

Also, there is something to keep in mind. All the functions that have as an argument or return value a pointer that points to data that is changed by the function (memset, strcpy, etc.) by default use generic pointers. But in the case of the pic14 port, qualifying them as __data pointers will result in optimized code size and speed. This is currently done ONLY for the malloc.h related functions, but it could be extended to all the functions that allow it. In fact this could also be extended to other ports/models that use a specific pointer in such cases.

With respect to aligned_alloc, the current pic14 custom version uses __data pointers instead of generic pointers. Apart of this, it is exactly the same as other generic ports, indeed in the fact that it doesn't provide an extern version of the function, thus not giving the possibility of getting a function pointer to it. There are notes about it in README.device-lib and README.support-regression. See also bug-2855.c

[Philipp Klaus Krause]

Patch pic14_src_regression:

AFAIK these pic-specific tests were created, since it seemed like the pic backends would be unable to use the normal regression testing system anytime soon. pic16 has been very close to beign able to use regression tests a few years ago (I don't know about the current situation, I guess it has regressed abit since). Now you make pic14 work.
So it seems to me, we could just remove the pic-specific test framework, and merge any tests that we want to keep into the generic regression test framework.

[salo]

I have not worked on the pic16 port, but for what I have seen there I suspect it would require some work similar to what I have done in the pic14 now. Maybe somehow easier ... or not.

Those tests are very simple, and the only thing that they provide over the generic tests is, precisely, this simplicity. They don't use the USART module of gpsim, for example. And they are single-file sources, with no more linkage requirements than libsdcc.

There is no need to integrate them with the generic testing system, but it doesn't hurt very much to leave them there. They are useful as a first step to start with, when nothing else works, as it is the case right now with the pic16 port.

[Philipp Klaus Krause]

pic16 should be much easier, as it already was very close to passing two years ago:
https://sourceforge.net/p/sdcc/feature-requests/403/

[Philipp Klaus Krause]

Patches pic14_support_regression and pic14_support_regression_tests:

I'm not sure about the split system yet. Could I get these patches without the split stuff?

[salo]

Yes, of course. If we agree on that later (see below), I will prepare two sets of patches: one with everything except the SPLIT system. And a second one with the SPLIT system, to be applied afterwards.

But I think first we should talk a bit about how to proceed in general and coordinate thinks to make it as easy as possible for everyone involved.

Please see comments to next post.

[Philipp Klaus Krause]

Patches pic14_debug* and pic14_fix*:

The debug system looks like a big change. Would it be possible to get the fixes without a dependency on the debug system?

[salo]

Please see comments to next post.

[Philipp Klaus Krause]

Thanks for all the work in those patches.
For years, it seemed like it is only a question of time until the pic backends get deprecated and removed. They still do have a substantial (but slowly declining) number of users, but there also are lots of bugs, and very little work was done on them. They have been "in development" for a very long time, and never fully passed the regression tests. And the pic14 situation was much worse than the pic16 one.
So getting pic14 to pass the regression tests is a huge improvement. I hope we can get everything cleared up and merged in time for the next release (we'd also need a gputils release with the necessary fixes there), so the next release of SDCC can have, for the first time, a stable pic backend.

[salo]

Thanks to you for your work here, Philipp.

I would like to suggest a possible way to proceed with these patches at the current moment.

I'm very sorry for this message being so long, but I think it will help.

Let me start with a short history of where all this comes from:

Oldest code in these patches (pcall for enhanced cores, IIRC) comes from the end of 2016.
At some points along 2017 and 2018 I wrote the remaining experimental code, and learnt SDCC in the process. At some moment I thought that it could be a good thing to publish it.
But I wanted to check that the code was really correct and it was not breaking something else, and the tests made so far by me (with real hardware) were not enough.
The regression tests of sdcc would have been the best for that but, unfortunately, they were useless for pic14.
So I decided to try to fix this, to be able to test everything against them, and this has been the last months of work.
The last phase has been moving it all to sdcc 3.9.0 (previous work was based on 3.8.0) and preparing it for publishing.

Good news: pic14 is ready for release. I know it, because I have checked it. Everybody can know it just taking sdcc 3.9.0, applying the patches, compiling and running the regression tests. (Well, they will have to do the same with gputils 1.5.0 in advance)

Bad news: I am human. I might have done something wrong. I have made all the tests in a single environment (my own computer) and other situations should also be checked, and could give different results.

And I would also find very useful if other people, more experienced on sdcc than me, take a look at the patches, more specifically at the sdcc issues than at the pic14 ones, and review what I have done there.

In general, the patches suppose a relatively big change in the pic14 backend but a very small change in the generic sdcc code. So I think integrating them with sdcc is a minor issue right now, but checking the work done in the pic14 backend (peer reviewing, testing, commenting, proposing changes, etc.) is more important.

I think that integrating the patches into the sdcc trunk should be the last stage, once they have been tested and it is clear what to include and what not to include. If there is something that must be checked, reviewed, changed, etc. it would be better to do it 'out' of the sdcc trunk.

So, what about creating a branch based on sdcc 3.9.0, apply the patches and give it some publicity, so everybody interested on it could check it and say what they have to say about it, if any? Is this possible? Is it a good idea?

I have no idea what could be the best way to proceed. I don't know how many people could become interested on this. By the activity I see in the sdcc site it seems that not too many.

So, Philippe, you know much more than me about all this, and I will follow your suggestions.

But I think that testing 'everything' I sent, as a whole thing, as the first step before deciding what to do next, will be less work for everybody (work to remove now and work to restore later). Doing it in a dedicated branch avoids the risk of adding to the trunk anything that could break it. Everything that I though it could be removed, I have removed it in advance. Everything included in the patches is because I though it should stay there.

And I have tried to be as careful as possible so, probably, everything could go to trunk more or less without changes. But, of course, you must see it in advance, and maybe the fastest and most secure way to do it is to leave the trunk apart now and check everything first in a dedicated branch.

Well, thanks for your patience with a so long message.

I expect your comment about all this, and thank you again for your work here.

[Philipp Klaus Krause]

Well, pic14 has few users (at least compard to mcs51, stm8, z80) - see also https://terminplaner4.dfn.de/xudoK5vzYi3oIX6O which means there will be few people to test and look at the branch. My guess would be that the best we can hope for is that someone tests the release candidate for pic14 before the final next release.

On the other hand, pic14 is a backend that is considered "in development", not stable, and does not currently pass regression tests. That gives a bit more freedom to make changes.

Considering that, I'd suggest a double approach:

• Pure fixes could go into trunk after review. To the point where most regression tests pass. So we could get a stable pic14 backend quite some time before the next release (even if a few tests would just be disabled in the end).
• The rest, such as the debug system, the regression test splitting stuff and optimizations could be evaluated in a branch or even multiple branches. I'd prefer to base the branch on trunk rather than 3.9.0 though. And there is the risk that in the end possibly no one but the two of us might look at the branch(es).

How far can we get with current gputils? You wrote a patch to gputils but I don't know how quickly it would be applied or how long it would take for a new gputils release after that patch is applied.

[salo]

Ok. I just want to get an idea in advance of what has to be done.

With respect to the SPLIT system, I will prepare a patch with the SPLIT system removed and the tests using it disabled for pic14 (lack of memory). I will also prepare another patch to append the SPLIT system afterwards.

With respect to debug, we can forget the four related patches by now, although this is not trivial.

That means removing their usage in the only one file that actually uses it: glue.c (a 90% rewritten file). The file ast.c (a 100% new file) also uses it, but it belongs to the experimental variadics code and can be ignored at the moment.

Apart from removing the debug messages in glue.c, I will have to rebuild the following five patches (the last four are almost exclusively dedicated to glue.c):

pic14_fix_regressions
pic14_fix_initvals
pic14_fix_bitfields
pic14_glue_ivals
pic14_glue_emitmaps

And finally update all the Makefile.am files that enable debug if requested, because otherwise sdcc will say '--debug-XXX option is unknown'.

Let's see what else is needed until we can pass the regression tests:

pic14_fix_regressions contains all the fixes needed to pass the regression tests, except the ones depending on glue.c and a (conceptually) single change made to gen.c to support multi-byte bitfields (found in pic14_fix_bitfields).

The following four patches could be replaced by a single one, because basically they are four steps in glue.c 'transformation' that end up in a full rewrite to make it similar to SDCCglue.c (only way I found to solve the remaining bugs after trying to do it based on the old code).

pic14_fix_initvals (minimal changes in ivals to fix character arrays)
pic14_fix_bitfields (minimal changes for multi-byte bitfields)
pic14_glue_ivals (full rewrite of ivals, removes previous changes)
pic14_glue_emitmaps (full rewrite of memmap emission)

For me, it will be much simpler to provide you a single patch with the final results and the changes you are requesting, than including those changes partially in every one of the four patches.

Once this is done, the regression tests will pass without errors, although some of them will be disabled by lack of functionality (experimental code) or lack of memory (split system).

In order to reach this point the way you want to follow, I would like to know in advance if there is something in those remaining five patches (from pic14_fix_regressions to pic14_glue_emitvals) that will have to be changed.

With respect to gputils, sorry that I didn't provide you the link before.
The patch request is here:

https://sourceforge.net/p/gputils/patches/74/

I don't see any release plans, and I would not count on them right now.

I do would count on having the patches included in the development trunk soon.

And maybe Molnár Károly will accept to schedule a release if requested, just to provide support to sdcc. But this should be requested once it is clear the situation here at sdcc, I think.

[Philipp Klaus Krause]

First, we need a fix for the out-of-tree-build regression that the pic14_device_lib patch introduced.
As of [r11268], in-tree builds of the pic14 library work, but out-of-tree-builds fail:

philipp@notebook5:~/sdcc-trunk/sdcc/build/device/lib/pic14$ LANG=C make
make  all-recursive
make[1]: Entering directory '/home/philipp/sdcc-trunk/sdcc/build/device/lib/pic14'
Making all in libc
make[2]: Entering directory '/home/philipp/sdcc-trunk/sdcc/build/device/lib/pic14/libc'
make[2]: *** No rule to make target 'isalnum.c', needed by 'libc_a-isalnum.o'.  Stop.
make[2]: Leaving directory '/home/philipp/sdcc-trunk/sdcc/build/device/lib/pic14/libc'
make[1]: *** [Makefile:436: all-recursive] Error 1
make[1]: Leaving directory '/home/philipp/sdcc-trunk/sdcc/build/device/lib/pic14'
make: *** [Makefile:376: all] Error 2

Then we could proceed with pic14_fix_regressions next. I've read through the patch, and did not notice issues (apart from wrong indentation off in gen.c from @@ -5176,6 +5299,12 @@ to @@ -5280,6 +5409,7 @@ and the #if 0 in ralloc.c at @@ -2473,11 +2474,12 @@ needing a comment on why the code was disabled). But I am not really familiar with the pic (neither backends nor architecture).

There also was a pic14 issue with inline functions in headers that needs to be resolved, see the changes in [r11268], which is a quick workaround to at least have the in-tree library build work.

[salo]

Hi, Philipp.

With respect to [r11268]:

inlined functions in pic14 are broken.
They will not work until the patches I sent (at least until pic14_glue_emitvals) are applied.

Once they work, device/include/pic14/stdlib.h will provide an inlined version (in lines 108-114) and an extern version (in 114-116) of aligned_alloc, ONLY for pic14 (lines 102-121), that return void __data*.

If the file is moved to device/lib, other ports will use lines 121-141, that contain the same as current device/lib/stdlib.h

All these line numbers refer to the version provided with the patches.

With respect to the out-of-tree build:

I have not tried it until now. I supposed you were not using VPATH at all.
I have found two issues:

FIRST ISSUE

device/lib/pic14/aclocal.am has a check with this contents (line 444 in sdcc 3.9.0):

if test "`cd $srcdir && pwd`" != "`pwd`"; then
  # Use -I$(srcdir) only when $(srcdir) != ., so that make's output
  # is not polluted with repeated "-I."
  AC_SUBST([am__isrc], [' -I$(srcdir)'])_AM_SUBST_NOTMAKE([am__isrc])dnl
  # test to see if srcdir already configured
  if test -f $srcdir/config.status; then
    AC_MSG_ERROR([source directory already configured; run "make distclean" there first])
  fi
fi

If the sources have been configured on-tree (so config.status is created) and then you try to build out-of-tree you will get that error.
This is a bug, but it will never happen if you always build out-of-tree.

SECOND ISSUE

The files

device/lib/pic14/libc/Makefile.am
device/lib/pic14/libm/Makefile.am

contain a line that says

SOURCE_DIRS = $(srcdir)

and should be replaced by (the current directory)

SOURCE_DIRS = .

And the files

device/lib/pic14/libsdcc/enhanced-no-xinst/Makefile.am
device/lib/pic14/libsdcc/enhanced/Makefile.am
device/lib/pic14/libsdcc/regular/Makefile.am

contain a line that says

SOURCE_DIRS = $(srcdir) $(srcdir)/..

and should be replaced by (the current directory and the parent directory)

SOURCE_DIRS = . ..

With this changes I have been able to build out-of-tree. YMMV.

The pic14 port now uses the generic source files from device/lib/ if none is found at its expected place under device/lib/pic14/, and at build time symbolic links are created if needed.

This fixes make those symbolic links be created in the build-tree instead of the source-tree, and VPATH finds them there.

WARNING

After applying the patch pic14_device_lib, that provides new and modified Makefile.am files, automake must be run (using device/lib/pic14/bootstrap.sh).

I don't have automake-1.16, so all my tests have been made with automake-1.15. Take this into account.

FINALLY

I'm very sorry for the inconveniences, Philipp. After going any further, let me insist (only once) to prepare everything in a branch, then sync the branch with the trunk, then test the synced branch, then sync the trunk with the branch (i.e. copy an already tested state). It is too much easy and less risky.

[Philipp Klaus Krause]

OK, let's continue in the new pic14 branch.

I'd suggest:
0) Fix the out-of-tree build in the branch, some testing on the branch
1) Merge the branch to trunk
2) Make regression tests work in the branch, some testing on the branch
3) Merge the branch to trunk
4) Further work in the branch

[Philipp Klaus Krause]

After step 3, we probably should look at all the open pic14-specific bug reports. Many of the bugs will have been fixed by step 3, but some might remain. Such remaining bugs could give direction on what to do first for step 4.

[Philipp Klaus Krause]

I implemented the changes you suggested in the pic14 branch. Now, it seems, both in-tree and out-of-tree builds are working.

Usually, I only do in-tree builds when testing something locally. However, the automated testing system (which does nightly builds of trunk and runs the regression tests for the stable ports) and releases use out-of-tree builds.

[Philipp Klaus Krause]

Could you have a look at the branch?
https://svn.code.sf.net/p/sdcc/code/branches/pic14/sdcc
Since for now, the branch only has the fix for out-of-tree-builds, it should be easy to test.