opcode.h includes architecture dependency in compile-once section

[srorso]

opcode.h includes the following in the compile-once section:

#ifndef _OPCODE_H   /* Line 7  */
#define _OPCODE_H

/* ... unrelated coding ...*/

    /* Program check if fpc is not valid contents for FPC register */     /* Line 539  */
#if defined(FEATURE_FLOATING_POINT_EXTENSION_FACILITY)          /*810*/
/* ... coding for use when feature defined ...*/
#else /*!defined(FEATURE_FLOATING_POINT_EXTENSION_FACILITY)*/   /*810*/
/* ... coding for use when feature not defined ...*/
#endif /*!defined(FEATURE_FLOATING_POINT_EXTENSION_FACILITY)*/  /*810*/

/* ... unrelated coding ...*/

#endif /*!defined(_OPCODE_H)*/

This code was added in commit 2c9aff9 on 1 Nov 2010, so it's been here a while without causing anyone any grief.

FEATURE_FLOATING_POINT_EXTENSION_FACILITY is architecture-dependent. Prior to coding for FPEF, the "coding for use when feature not defined" would be independent of the architecture and placed well in opcode.h.

The present coding, located where it is, would only be generated for the first archmode being compiled (usually 370), and that generated code, a macro with FPEF unavailable, would be used for all architectures. Even for those architectures that include FPEF.

I found the issue because LFPC, Load Floating Point Control, failed with a specification exception when the BFP rounding mode was a 7. 7 is not a valid rounding mode unless the FPEF is installed. MSVC compiler option /P to look at the preprocessor output really helped to figure out what was happening. And it didn't hurt that I did the same thing in ieee.c with code that I thought should be architecture-independent...but wasn't.

Interestingly, SRNMB with rounding mode of 7 works just fine.

This issue affects the following instructions:

LOAD FPC  (esame.c)
SET FPC (esame.c)
LOAD FPC AND SIGNAL (dfp.c)
SET FPC AND SIGNAL (dfp.c)

All four instructions are identified in SA22-7832-10 (POP) as Floating Point Support instructions; their location in Hyperion modules seems to be based on when the emulation code was written.

I will move the code out of the compile-once section and add the needed #undef just before it. Since only BFP is affected, I do not see a need to commit prior to the commit for Softfloat 3a.

[Fish-Git]

Excellent catch, Steve! Your description/analysis of the problem is spot on!

I suspect there may well be similar/identical issues for other FEATUREs in other areas of Hercules.

One which springs to mind (which I believe has since been fixed) is one which either Jürgen or Peter found (I forget who it was (sorry whoever it was!)) related to FEATURE constants which start with an underscore as opposed those which don't start with an underscore that was related to storage keys I believe (I can't remember).

FEATURE constants which do not start with an underscore (e.g. FEATURE_ASN_AND_LX_REUSE) are per-architecture #defines (i.e. if the constant is #defined for the given build architecture (e.g. 370, 390 or 900) then that architecture needs that feature) whereas FEATURE constants that do begin an underscore (e.g. _FEATURE_ASN_AND_LX_REUSE) get #defined (by featchk.h) when any of the build architectures (370/390/900) need a given feature.

Which constant you should check for (#ifdef) in your code depends on whether your code is architecture dependent code or not. If it's not architecture dependent code but the code needs to exist because at least one of the build architectures needs it, then you need to check the _FEATURE_XXX underscore constant (such as the alrf_cmd function in hscpufun.c).

In architecture dependent code however (such as the ESAIR instruction in control.c) you need to check the non-underscore constant (since that varies by build architecture).

Issues such as this as well as the issue you yourself discovered are subtle idiosyncrasies of Hercules that even seasoned developers sometimes forget about and trip over.

We've talked about (and I believe Harold even took a stab at trying to start one) the need for some type of "Hercules Internals" manual that documents such things, but unfortunately one does not yet exist.

As for myself I've taken to adding new pages to our GitHub repository's "Wiki" here and there whenever I can. I encourage all of our current developers to do the same. Such is certainly better than nothing, which is almost what we've got at the moment.

But yeah, your analysis and proposed fix to the issue you described is precisely correct. You are proving (at least to me!) to be an extremely valuable member of the Hercules team! I am continually impressed with your work! Keep it up! :)

[srorso]

To all:

The correction (move the code, add #undef) works fine. I will commit in due course with the Softfloat 3a implementation. If there is a need, I can isolate the change and commit sooner.

To Fish:

Many thanks for the kind words, and for the primer on the feature variables. I saw some with a leading "_" but did not appreciate the significance.

[srorso]

Issue addressed in commit 83f60bf in December, 2016, as part of the SoftFloat 3a For Hercules integration.