#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.

[salo]

Hi, Philipp.

I have tested the new pic14 branch. This is the first time I actually work with svn. This is what I have done:

$ mkdir -p /public/salo/pic/sdcc-svn/new
$ cd /public/salo/pic/sdcc-svn/new
$ svn checkout https://svn.code.sf.net/p/sdcc/code/branches/pic14/sdcc .

Revisión obtenida: 11272

$ cd ..
$ mv new r11272
$ mkdir build
$ cd build
$ ../r11272/configure 2>&1 | tee log.configure
$ make 2>&1 | tee log.make

$ ls -Al device/lib/build/pic14
total 2360
-rw-r--r-- 1 salo salo 614202 jun 12 04:04 libce.lib
-rw-r--r-- 1 salo salo 631060 jun 12 04:04 libc.lib
-rw-r--r-- 1 salo salo 296989 jun 12 04:04 libme.lib
-rw-r--r-- 1 salo salo 315489 jun 12 04:04 libm.lib
-rw-r--r-- 1 salo salo 248152 jun 12 04:04 libsdcce.lib
-rw-r--r-- 1 salo salo 271627 jun 12 04:04 libsdcc.lib

The sizes are more or less as expected.

The first time I tried I found two errors:

ERROR in device/lib/pic14

It was taking all the source files from the generic directory $(top_srcdir)/device/lib, and none from the pic14 directories.

The file device/lib/pic14/Makefile.common must be changed this way:

 # NOTE: last empty line required because foreach concatenates several copies of this text
 define link-generic-files
 $(1)/%.c: $$(GENERIC_SRC_DIR_ABS)/%.c
-   test -e $$@ || ln -s $$< $$@
+   test -e $$(srcdir)/$$@ || ln -s $$< $$@

 endef

So now the symbolic link is created in the build tree if the source file is not present in the source tree (of course, none is present in the build tree).

ERROR in device/lib/pic16

During 'make' (not during configure) it required aclocal-1.16 (I'm not absolutely sure if it was actually aclocal or any other automake tool). I think this should not happen.

SOLUTION

Anyway, I run bootstrap.sh in pic16 (so it changed to automake-1.15) and also in pic14 (because I had changed Makefile.common, which is included by the Makefile.am files inside device/lib/pic14).

This time the build went ok, but I have no way to test the libraries. They will probably be very buggy (at least the libc ones) because the backend doesn't have already any fix applied. But at least they build.

Also, keep in mind that I am using patched versions of gputils, but this doesn't affect the libraries, but only some regression tests. I will identify them at some moment so we can disable them until gputils gets patched.

[salo]

Hi, Philipp.

In order to leave the trunk in a sane state, you should make one more change.

In device/lib/pic14/configure.ac you will find the following lines. They control which library flavours are built:

# default values for the following conditions
: ${DEBUG:=0}
: ${NOOPTS:=1}
: ${EXPERIMENTAL:=1}

You should change EXPERIMENTAL to 0 (zero), otherwise sdcc will give an error when called with experimental options enabled, as they are not available yet.
Be sure DEBUG is also 0. NOOPTS doesn't hurt, but I would set it to 0, too. There is no sense right now to detect optimization bugs. This way only the regular and enhanced flavours of the libraries will be built.

[Philipp Klaus Krause]

Done in the pic14 branch. The warnings about unrecognized options are indeed gone now. There are still many variants being built, though:

~~~~
philipp@notebook5:~/sdcc-branches/pic14/sdcc$ ls -l device/lib/build/pic14/libc*
-rw-r--r-- 1 philipp philipp 614202 Jun 11 19:40 device/lib/build/pic14/libce.lib
-rw-r--r-- 1 philipp philipp 634000 Jun 11 19:40 device/lib/build/pic14/libceo.lib
-rw-r--r-- 1 philipp philipp 634094 Jun 11 19:40 device/lib/build/pic14/libceox.lib
-rw-r--r-- 1 philipp philipp 614296 Jun 11 19:40 device/lib/build/pic14/libcex.lib
-rw-r--r-- 1 philipp philipp 631060 Jun 11 19:40 device/lib/build/pic14/libc.lib
-rw-r--r-- 1 philipp philipp 650306 Jun 11 19:40 device/lib/build/pic14/libco.lib
-rw-r--r-- 1 philipp philipp 650400 Jun 11 19:40 device/lib/build/pic14/libcox.lib
-rw-r--r-- 1 philipp philipp 631154 Jun 11 19:40 device/lib/build/pic14/libcx.lib
~~~

[salo]

Probably they were built before you disabled NOOPTS and EXPERIMENTAL. After disabling those options the clean target will ignore them and so they will not be removed.

The libraries are built under the device/lib/pic14 directory and afterwards copied (and renamed) to device/lib/build/pic14. If they don't get removed from their original place they will be copied again.

[Philipp Klaus Krause]

On a fresh checkout of the pic14 branch, the build now seems to work as expected for me, both in-tree and out-of-tree.

Any remaining problems that need to be fixed before merging to trunk?

[salo]

Hi, Philipp.

I have tested all again. The issue I reported in a previous post, related to pic16 (automake required when running make) is already present, both in the pic14 port and in the pic16 port. There is also a minor issue with pic14 (it unnecessarily rebuilds some files in some conditions).

This is too large to explain in detail here, so I attach a report of all this.

All these issues are not new and they are already present in the trunk, so I would merge to trunk to, at least, remove the problems introduced by pic14_device_lib.

Philipp, keep in mind that there are a lot of failures related to pic14 and pic16, and also some failures on the generic sdcc build system. If we start looking at them now we will never end.

[Philipp Klaus Krause]

Well, the pic ports are weird. Having their own library implementation (now mostly fixed for pic14) and the weird build system are just aspects of that. Maybe we can get to that later, once regression tests pass.

[Philipp Klaus Krause]

It worked for you and me, and is now merged to trunk.
If we introduced any big issues, we will notice red dots at http://sdcc.sourceforge.net/snap.php tomorrow.

So the next step will be making regression tests pass.

Philipp

[Diego Herranz]

Thank you very much for all the work on this to both of you.

I had given up any hope of seeing stable pic ports but we may be closer than ever.

I don't normally use the pic14 port but I've been using the pic16 port for some time and if you implement some changes, I can help testing my code to make sure everything works OK still.

Thanks!

[salo]

Thank you, Diego.

It's nice to know that 'we are not alone' :-)

[salo]

Hi, Philipp.

I start a 'top level' thread to keep thinks separated.
I've been checking the current state of the pic14 branch.

These is the situation at [r11277]:

* The branch is in sync with trunk.
* Patch sdcc_generic_fixes already applied [r11257]
    The next three files have been finally excluded:
        device/lib/mbstowcs.c
        support/regression/tests/bitfields.c
        support/regression/tests/gcc-torture-execute-loop-13.c
* Patch pic14_define_enhanced already applied [r11258]
* Patch pic14_device_lib already applied [r11259]
    The new file device/include/pic14/stdlib.h has been changed afterwards [r11268] (to be reverted later)
    The file device/lib/pic14/configure.ac has been changed to disable building the extra library flavours [r11272]
    The following files have been changed to fix the build of the device library out of the source tree [r11270]:
        device/lib/pic14/libc/Makefile.am
        device/lib/pic14/libm/Makefile.am
        device/lib/pic14/libsdcc/enhanced-no-xinst/Makefile.am
        device/lib/pic14/libsdcc/enhanced/Makefile.am
        device/lib/pic14/libsdcc/regular/Makefile.am
    and also [r11274]
        device/lib/pic14/Makefile.common

So far, so good.

Looking at the differences between the current state and that at sdcc 3.9.0 I think that all the remaining patches will apply without failures except pic14_support_regression_tests, that will fail with 64 test files that will require manual merging.

I'm gonna prepare this manual merging in advance. I did it with 171 files when upgrading the patches from sdcc 3.8.0 to sdcc 3.9.0, so I'm somewhat skilled at managing diff3 right now.

Until I have it, I would suggest you to try the following:

STEP 1. Forget pic14_device_lib_startup (this is the easiest step).

STEP 2. Take a look at pic14_src_regression, apply it and test it.

If we find problems with gpsim, this will be the best place to fix them.

Take a look at src/regression/Makefile. You will see that VERBOSE and DEBUG are disabled by default.

The line that assigns to TARGETS currently has all pic14 flavours enabled and pic16 disabled.
Change it like this so only the pic14 and pic14e flavours are tested by default:

    # List of targets
    # You can prefix any target name with a dash to disable it
    TARGETS ?= $(sort $(filter pic%, pic14 pic14e -pic14o -pic14eo -pic14x -pic14ex -pic14ox -pic14eox -pic16 ))

After building, change dir to src/regressions and run 'make' without arguments. You will get help and see which targets are enabled. The Makefile can be changed on-the-fly and doesn't need a rebuild to run again.

WARNING: I have not tried this in an out-of-tree build, but I think it will work. Check both cases.

STEP 3. Next step will be to do the same with pic14_support_regression

There is no need to disable the extra flavours if you avoid the use of make without arguments. Just run 'make test-pic14e' or 'make test-mcs-small', etc.

But to be sure, you can change as follows support/regression/Makefile (reconfiguring or rebuilding will overwrite it, as it is based in a Makefile.in):

This is how it looks now:

############################################################################
# WARNING: ALL_PORTS now INCLUDES the PIC14 ports
############################################################################
ALL_PORTS = $(filter-out .svn rrz80 rrgbz80 pic16 pic14-common mcs51-common,$(notdir $(wildcard $(PORTS_DIR)/*)))
# This is independent of the previous line
PIC14_PORTS = $(filter-out pic14-common,$(notdir $(wildcard $(PORTS_DIR)/pic14*)))

If, in the line ALL_PORTS, you replace 'pic14-common' with 'pic14%' the whole pic14 port will be disabled by default (so 'make' without arguments will run the tests 'as usual'), but you can already test the pic14 port running 'make test-pic14e' (one flavour) or 'make test-pic14-all' (all flavours, but experimental ones will fail because doesn't exist yet. It should fail with around 2000 compiler errors per flavour, but nothing worse :-)

Here the first and most important thing to do is to check that the other ports are not affected but the changes introduced into the generic test system.

WARNING: not tested out-of-tree

LAST NOT LEAST. More than enough for now. At this stage we will be confident that nothing breaks other ports, and we will see which is the current state of the pic14 port.

My old notes say that we will find something like this:

    # pic14         : 219 compiler errors, 153 linker errors, 11 exceptions, 4 timeouts, 874 failures, 6117 tests, 1953 test cases
    # pic14+exp     : 219 compiler errors, 153 linker errors, 11 exceptions, 4 timeouts, 875 failures, 6117 tests, 1953 test cases
    # pic14-opt     : 219 compiler errors, 220 linker errors, 10 exceptions, 3 timeouts, 760 failures, 5757 tests, 1939 test cases
    # pic14-opt+exp : 219 compiler errors, 220 linker errors, 10 exceptions, 3 timeouts, 760 failures, 5757 tests, 1939 test cases
    # pic14e        : 219 compiler errors, 138 linker errors, 10 exceptions, 3 timeouts, 884 failures, 6229 tests, 1953 test cases
    # pic14e+exp    : 219 compiler errors, 138 linker errors, 10 exceptions, 3 timeouts, 884 failures, 6229 tests, 1953 test cases
    # pic14e-opt    : 219 compiler errors, 141 linker errors, 36 exceptions, 3 timeouts, 891 failures, 6140 tests, 1951 test cases
    # pic14e-opt+exp: 219 compiler errors, 141 linker errors, 36 exceptions, 3 timeouts, 891 failures, 6140 tests, 1951 test cases

[Philipp Klaus Krause]

How about a variant of pic14_support_regression and pic14_support_regression_tests that does not introduce the split system, and introduces only the pic14 and pic14e test targets?

Philipp

[Philipp Klaus Krause]

When I apply pic14_src_regression to the pic14 branch or trunk, everything fails with

gplink: invalid option -- 'C'

and is thus worse than before.

[salo]

$ gplink --version
gplink-1.5.0 #1285 (May 29 2019)

$ gplink --help
Usage: gplink [options] [objects] [libraries]
Options: [defaults in brackets after descriptions]
  -a FMT, --hex-format FMT       Select hex file format.
  -b OPT, --optimize-banksel OPT Remove unnecessary Banksel directives. [0]
  -B, --experimental-banksel     Use experimental Banksel removal.
  -c, --object                   Output executable object file.
  -C, --no-cinit-warnings        Disable this warnings of _cinit section with -O2 option:
                                   "Relocation symbol _cinit has no section."
  -d, --debug                    Output debug messages.
  -f VALUE, --fill VALUE         Fill unused program memory with value.
  -h, --help                     Show this usage message.
  -I DIR, --include DIR          Specify include directory.
  -j, --no-save-local            Disable the save of local registers to COD file.
  -l, --no-list                  Disable list file output.
  -m, --map                      Output a map file.
      --mplink-compatible        MPLINK compatibility mode.
  -o FILE, --output FILE         Alternate name of output file.
  -O OPT, --optimize OPT         Optimization level. [1]
  -p OPT, --optimize-pagesel OPT Remove unnecessary Pagesel directives. [0]
  -P, --experimental-pagesel     Use experimental Pagesel removal.
  -q, --quiet                    Quiet.
  -r, --use-shared               Use shared memory if necessary.
  -s FILE, --script FILE         Linker script.
  -t SIZE, --stack SIZE          Create a stack section.
  -S [0|1|2], --strict [0|1|2]   Set the strict level of the missing symbol.
                                     0: This is the default. No message.
                                     1: Show warning message if there is missing symbol.
                                     2: Show error message if there is missing symbol.
      --strict-options           If this is set, then an option may not be parameter
                                   of an another option. For example: -s --quiet
  -u, --macro symbol[=value]     Add macro value for script.
  -v, --version                  Show version.
  -w, --processor-mismatch       Disable "processor mismatch" warning.
  -W, --experimental-pcallw      Remove unnecessary PCALLW stubs created by SDCC.

Default linker script path /usr/share/gputils/lkr
Default library path /usr/share/gputils/lib

Report bugs to:
<URL:http://gputils.sourceforge.net/>

[Philipp Klaus Krause]

I'm using gputils 1.4.0, as that is the latest version commonly available in distributions (the only noteable exception, that is at 1.5.0 is FreeBSD). I'd prefer if most stuff will work using gputils 1.4.0.

[salo]

The -C option of gplink just disables some warnings, so removing it from the Makefiles will do:

$ grep -r  'Wl.*-C' sdcc-pic14-bundle/sdcc-patches/
sdcc-pic14-bundle/sdcc-patches/pic14_src_regression:+CFLAGS += -Wl,-C,-l,-O2
sdcc-pic14-bundle/sdcc-patches/pic14_support_regression:+LINKFLAGS += -Wl,-C,-l,-O2

In src/regression/Makefile you will find:

# linker options (for sdcc)
CFLAGS += -Wl,-C,-l,-O2
CFLAGS += --nostdlib -L$(top_builddir)/device/lib/build/$(ARCH) -L$(top_builddir)/device/non-free/lib/build/$(ARCH)

and in support/regression/ports/pic14-common/spec.mk:

SDCCFLAGS += -mpic14 -p$(DEV) --less-pedantic
SDCCFLAGS += --no-warn-non-free
# At least the -O2 option must be given to gplink to discard unused symbols from executable
LINKFLAGS += -Wl,-C,-l,-O2
# The libraries to link
LIB_SUFFIX = $(LIB_E)$(LIB_O)$(LIB_X)
LINKFLAGS += pic$(DEV).lib libsdcc$(LIB_SUFFIX).lib libc$(LIB_SUFFIX).lib libm$(LIB_SUFFIX).lib

[Philipp Klaus Krause]

pic14_fix_regressions has been applied to the pic14 branch a while ago. Today, I picked parts of pic14_support_regression_tests into the pic14 branch.

Unfortunately, for me, running test-pic14 leaves the console unuseable in the end (don't see what I type, same problem in trunk). It is not a big issue, but it would be good to have it fixed before proceeding, so we can reuse the same terminal window for multiple runs.

[salo]

Hi, Philipp.

The issue with the console also happens to me. My guess is that it is related to gpsim leaving the console in an abnormal state after being killed by timeout. Once timeouts disappear, this issue also disappears.

Running (blindly) 'reset' will restore the console to its default state. reset is provided by ncurses-bin on Debian based systems. Maybe dettaching gpsim from the console when running it wil fix this issue. Not tried, and probably very dependant on the underlying architecture (Linux, Windows, etc.)

[Philipp Klaus Krause]

Could you check if this is really from gpsim (as opposed to infrastructure in sdcc), and if yes, file a bug report with gputils?

[salo]

gpsim is not part of gputils. It is another project.

At some point in the future I will try to isolate this problem and check it. And all issues related to gpsim will be reported. In any case they will have to be checked with the latests gpsim release, that is coming right now (there is currently a release candidate for 0.31.0).

Anyway, I don't see how could this be related to sdcc, although it could be an issue related to fwk/lib/timeout.c A report about what happens in Windows could be useful.

[salo]

Hi, Philipp.

Confirmed it is related to timeout killing gpsim. It can be fixed this way:

--- r11278-svn/support/regression/ports/pic14/spec.mk   2019-06-20 10:32:47.000000000 +0200
+++ r11299/support/regression/ports/pic14/spec.mk   2019-06-25 06:33:23.852623562 +0200
@@ -52,9 +52,9 @@
 # run simulator with SIM_TIMEOUT seconds timeout
 %.out: %$(BINEXT) $(CASES_DIR)/timeout
    mkdir -p $(dir $@)
-   -$(CASES_DIR)/timeout $(SIM_TIMEOUT) $(GPSIM) -i -s $< -c $(PORTS_DIR)/pic14/gpsim.cmd > $@ || \
+   -$(CASES_DIR)/timeout $(SIM_TIMEOUT) $(GPSIM) -i -s $< -c $(PORTS_DIR)/pic14/gpsim.cmd > $@ < /dev/null || \
      echo -e --- FAIL: \"timeout, simulation killed\" in $(<:$(BINEXT)=.c)"\n"--- Summary: 1/1/1: timeout >> $@
    $(PYTHON) $(srcdir)/get_ticks.py < $@ >> $@
    -grep -n FAIL $@ /dev/null || true

 _clean:

[salo]

Hi, Philipp.

May I have some info about the pic specific regression tests?

I'd like to know about your results with pic14_src_regression. What gputils version and gpsim version did you tried? What are you using in the snapshots? Did all tests pass? Which ones failed? Any error message? Warnings? Did you tested both pic14 and pic14e?

If gplink emits the annoying warning about _cinit, it can be filtered out in the Makefile, but I will need to know exactly the text to be filtered out.

This is the first part of testing which versions of gputils and gpsim are required.

The second one consists on doing the same with the generic regression tests, after pic14_support_regression (not the tests, but the infrastructure) is applied. It requires some features of gpsim that maybe are not available on old versions. In fact, gpsim 0.30.0 has a bug that requires a workaround in support/regression/ports/pic14-common/support.c to make it work. We should check this with the versions you intend to use in the snapshots.

[Philipp Klaus Krause]

Current ([r11299])situation for me in the pic14 branch (using gputils 1.4.0, gpsim 0.30.0) for pic-specific tests.

[salo]

Hi, Philipp.

For your information, I attach my log using gputils 1.5.0, based on r11299.

[Philipp Klaus Krause]

Current ([r11299])situation for me in the pic14 branch (using gputils 1.4.0, gpsim 0.30.0) for normal tests.