hercules-390 / hyperion

Hercules 390
Other
251 stars 70 forks source link

configure.ac selects invalid arguments for gcc -mtune= and -mcpu= options on ARM #233

Closed drboone closed 6 years ago

drboone commented 7 years ago

configure.ac seems to use the output of uname -m as the arguments for the gcc tuning options. On at least some ARM systems (in my case a Beaglebone Black), uname -m returns 'armv7l', which is valid in a system triple, but not for the tuning options. A list of valid options appears here:

https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html

but I suspect that it's easier to just use the value 'native' for those options.

arm_mtune_mcpu.diff

srorso commented 7 years ago

Hi Mr. Boone:

(First names are common and accepted in this issue list, but I defer to your sensibilities and preferences.)

What happens when you build Hercules using CMake? The CMake build uses a different process to develop optimization flags, with specific allowances for ARM. Although I must admit I do not have an ARM-based system, so I have not tested the process, and I have yet to test the CMake build on QEMU.

You don't mention the OS/version you are running, but it appears that Beaglebone Black uses a Debian port. Debian 9 (Stretch) has a good level of CMake in its repositories. CMake 3.6 has been backported to Debian 8 (Jessie). CMake 3.0 is included in base Debian 8 and is too old. I have built CMake from source on several systems and it is not difficult. README.rst in the CMake source distribution has instructions. CMake 3.4.3 is the minimum version for a Hercules CMake build.

And for both CMake and autotools (configure.ac), you can override the optimization options with your own string as follows:

configure.ac: <source-dir>/configure --enable-optimization="<optimization-string>"

CMake: -DOPTIMIZE="<optimization-string>"

Either of the above beats hacking the generated build files. For CMake, -DOPTIMIZE must be upper case. <optimization-string> should have the casing required by the compiler, for example:

-DOPTIMIZE="-O2 -march=native"
--enable-optimize="-O2 -march=native"

ARM is not an "officially" supported processor for Hercules, and the above may not help at all, but I am eager to see your results.

Best Regards, Steve Orso

drboone commented 7 years ago

Steve,

The platform is, as mentioned, a Beaglebone Black. OS is Debian stretch. Gcc is 6.3.0. Cmake is 3.7.2. The objective is to have a little portable demo system for a number of emulators, including Hercules. I had just hacked the generated makefiles, and got an apparently working binary. It didn't occur to me at the time that the --enable-optimize method would override the problem, or I would have done that.

I tried a build with CMake and make. That avoided the gcc optimization error, and produced a binary that at least launches. I do get about a million warnings, as opcode.h seems to use the function attribute 'regparm' which gcc on ARM doesn't like, and opcode.h is included in a lot of places.

I'm now running a build using the --enable-optimize route with 'configure'. I think this has successfully overridden the default settings from configure.ac, but it'll be a couple of hours before the build completes, so I'll have to confirm that in the morning.

I do think it'd be nice if we could fix the configure.ac defaults for the next guy. "native" may not be optimal (among other things, it assumes you're building on the target platform), but it's better than compiler errors. That segment of code is specific to xscale and ARM, so it's not like it'd be breaking another platform.

/Dennis

srorso commented 7 years ago

Hi Dennis:

Thanks for giving the CMake build a try.

The gcc compiler on ARM does not accept the regparm attribute; the CMake build should not have included it. There is a test in the CMake build for a working regparm attribute. It appears, however, that that gcc just issues a warning , and test does not include -Wall, so a warning that regparm will be ignored does not cause the build to exclude it.

If you are willing, would you please run the following two tests from a working directory you can later delete and post the console output:

cc <hercules_source_dir>/CMake/CMakeHercTestRegparm3.c -O3 -fomit-frame-pointer
./a.out
cc <hercules_source_dir>/CMake/CMakeHercTestRegparm3.c -O3 -fomit-frame-pointer -Wall -Werror
./a.out

The test program is very short and will verify that -Wall -Werror will address the warnings you received.

There are five places in the CMake build where program execution is used to probe the target system; this creates cross-compiling issues that remain to be addressed. The autotools build has three such cases.

I agree, changing the xscale-yes|arm*-yes) leg of the optimization case statement in configure.ac seems simple enough. But I have backported enough stuff into the autotools build that I fear breaking something else, a fear amplified by the absence of a real ARM system to test the build on. So I will avoid the seductive "one-line-change" that turns into something that looks like "Fatal Attraction" and continue working on replacing the Hercules autotools build with a CMake build.

The best way to address this (CMake or autotools) would be to develop a mapping of uname -m results to the ARM processor family; armv7l then becomes the gcc-recognized armv7-a.

I find your portable emulator setup very interesting. If I may, which other emulators join Hercules on your Beaglebone?

Best Regards, Steve Orso

drboone commented 7 years ago

Steve,

Looks like -Wall -Werror does address the regparm problem. Console log attached. I suspected that as a warning, that was eluding detection.

So far I've built simh, dtcyber, hercules, and dps8m simulators. Much testing remains, and I have no idea how much (little?) performance to expect from any of them. Two of us will have some variant of this stuff at VCFmw in a couple of weeks.

De

wallwerrortest.txt

srorso commented 7 years ago

Hi Dennis:

Many thanks for running the sample and providing the results. The quick fix would be to add -Wall -Werror to COMPILE_DEFINITIONS in the following starting at line 550 of Herc00_Includes.cmake.

try_run(RUN_RESULT_VAR COMPILE_RESULT_VAR
        ${PROJECT_BINARY_DIR}
        SOURCES ${PROJECT_SOURCE_DIR}/CMake/CMakeHercTestRegparm3.c
        COMPILE_DEFINITIONS "-O3 -fomit-frame-pointer"
        COMPILE_OUTPUT_VARIABLE R3_CompileOutput
        RUN_OUTPUT_VARIABLE R3_RunOutput
        )

But further research shows that the run-time test is needed only when building with gcc 3.2.x on Cygwin. Cygwin is no longer a supported build environment for Hercules, so this runtime test can be replaced by a compile-only test. Because either change requires the same regression testing (lots of builds), I will pursue the compile-time test.

Thank you also for trying the CMake build on ARM. I am quite happy with the results you reported. Did your autotools-based build with --enable-optimize="" work as well?

As for your emulator list, quite impressive. I knew about simh (I have IBM 1130 experience), but the others are new to me. Many thanks for sharing the list.

Best Regards, Steve Orso

srorso commented 7 years ago

Hi Dennis:

I have attached a pre-commit correction to the CMake issue you encountered. It has been regression tested on Debian 9-32 bit and Debian 9-64 bit (Intel processor). While I continue regression testing on other platforms, would you be willing to test on your Beaglebone? A Hercules build is not required to test, although you are certainly welcome to do so.

  1. Untar the attached files into the Hercules source directory subdirectory CMake.
  2. From an empty build directory, re-run CMake as you did before.
  3. When CMake completes, in the build directory, grep REGPARM config.h

You should see HAVE_ATTR_REGPARM #undef'd and surrounded by comments. If you see it defined to 1, then the new code needs some work.

Re the requirement for an empty build directory: when CMake is re-run as a result of changes/corrections in the CMake scripts, the contents of the CMakeFiles directory and the CMakeCache.txt files roundly confuse CMake. While you can just delete those two files, there is not much left in the build directory that will not be rebuild, so I normally trash everything, including the hidden subdirectories.

Thanks in advance for any assistance you are able to provide.

Best Regards, Steve Orso

cmakefix.tar.gz

drboone commented 7 years ago

Steve,

I think we have success, then.

/ #undef HAVE_ATTR_REGPARM /

Actual build is running now, with no REGPARM errors.

De

srorso commented 7 years ago

Hi Dennis:

That is wonderful news; thanks for giving that a try...and doing the build.

To give myself some assurance of success, I ran a build on QEMU (armv6l, Raspian, gcc 4.9.2). Took about two and half hours, but matched your results. I even IPL'd DOS/360 on Hercules running on QEMU, for two layers of emulation.

Having finished regression testing (builds on 14 virtual systems), functional testing on QEMU and getting your feedback, I will commit the permanent fix to the CMake build within a day or so.

And thanks again for reporting this issue!

Best Regards, Steve Orso

srorso commented 7 years ago

Correction for this issue is in commit 1b43944.