Issues related to compiling AmberTools with clang-16

[dacase]

These errors are seen (in old,old,old xleap code):

These are the errors from the osx-64 and osx-arm64

2023-10-06T01:41:17.8020210Z /Users/runner/miniforge3/conda-bld/ambertools_1696552439471/work/AmberTools/src/leap/src/Wc/WcActCB.c:316:5: error: incompatible function pointer types passing 'void (Widget, XtGrabKind)' (aka 'void (struct _WidgetRec *, XtGrabKind)') to parameter of type 'PFVWidInt' (aka 'void (*)(struct _WidgetRec *, int)') [-Wincompatible-function-pointer-types]
2023-10-06T01:41:17.8021260Z                          XtPopup, XtGrabNone );
2023-10-06T01:41:17.8022180Z                          ^~~~~~~

2023-10-06T01:41:17.8025180Z /Users/runner/miniforge3/conda-bld/ambertools_1696552439471/work/AmberTools/src/leap/src/Wc/WcActCB.c:323:5: error: incompatible function pointer types passing 'void (Widget, XtGrabKind)' (aka 'void (struct _WidgetRec *, XtGrabKind)') to parameter of type 'PFVWidInt' (aka 'void (*)(struct _WidgetRec *, int)') [-Wincompatible-function-pointer-types]
2023-10-06T01:41:17.8026070Z                          XtPopup, XtGrabExclusive );
2023-10-06T01:41:17.8026340Z                          ^~~~~~~

2023-10-06T01:41:17.8103750Z /Users/runner/miniforge3/conda-bld/ambertools_1696552439471/work/AmberTools/src/leap/src/Wc/WcActCB.c:916:23: error: incompatible function pointer types passing 'void (Widget, const char *, XtCallbackList)' (aka 'void (struct _WidgetRec *, const char *, struct _XtCallbackRec *)') to parameter of type 'AddOrRemoveProc' (aka 'void (*)(struct _WidgetRec *, char *, struct _XtCallbackRec *)') [-Wincompatible-function-pointer-types]
2023-10-06T01:41:17.8104400Z                                 "WcAddCallbacks", XtAddCallbacks );
2023-10-06T01:41:17.8104690Z                                                   ^~~~~~~~~~~~~~

2023-10-06T01:41:17.8107390Z /Users/runner/miniforge3/conda-bld/ambertools_1696552439471/work/AmberTools/src/leap/src/Wc/WcActCB.c:926:26: error: incompatible function pointer types passing 'void (Widget, const char *, XtCallbackList)' (aka 'void (struct _WidgetRec *, const char *, struct _XtCallbackRec *)') to parameter of type 'AddOrRemoveProc' (aka 'void (*)(struct _WidgetRec *, char *, struct _XtCallbackRec *)') [-Wincompatible-function-pointer-types]
2023-10-06T01:41:17.8107970Z                                 "WcRemoveCallbacks", XtRemoveCallbacks );
2023-10-06T01:41:17.8108230Z                                                      ^~~~~~~~~~~~~~~~~

I don't have access to OSX machines, but have installed clang-16 on my linux (Ubuntu) box, and will see if there is some simple fix. I don't think anyone understands what this code is doing.

Going forward, AmberTools has a significant number of function declataions without prototypes (cpptraj, etc.). These will need to be addressed soon.

[dacase]

Update:

1. I can reproduce the same errors (plus some additional ones) using clang-16 on Linux. But no fix is obvious to me -- code makes assumptions about passing void pointers around that apparently clang-16 no longer accepts. [My ignorant query: is gcc disallowed/unfavored for OSX compilation?]
2. A workaround: set -DBUILD_GUI=FALSE in the cmake invocation, when clang is the compiler. This would have a minimal impact on users: the xleap GUI still has some specific uses, but probably only for those of us who learned it a long time ago. The learning curve for the GUI part is steep. So, the impact on users should be small if we skip building this.

[mattwthompson]

Are there other GUI programs bundled with AmberTools? I know that some people strongly prefer using xleap but my organization doesn't need it (and I on a personal level have some strong anti-GUI opinions).

[h-vetinari]

My ignorant query: is gcc disallowed/unfavored for OSX compilation?

Yes, we only have one compiler stack per platform, to avoid various ABI issues and keep our CI matrix sane

[h-vetinari]

The issue is not just that clang doesn't accept certain constructs by default anymore, it's also that the C standard itself is getting stricter about the things that are allowed, and the compiler authors follow that. So if you have ancient code you rely on, the options are:

• [good] fix the code
• [ok] try to use an old C-standard version
• [ok-ish] turn off the warnings where possible

Though the best might actually be to switch the GUI to a more modern stack (like wayland etc.) than X, which is just bitrotting in ever-escalating fashion. I realize that that's probably not a small operation though, so if there are few users of the GUI, -DBUILD_GUI=FALSE might also be a feasible solution.

[dacase]

FWIW: I tried adding -std=c89 to the CFLAGS option, but still got the same errors in compiling the xleap code.

Matt asked about other GUI programs. The only one I know if is xparmed, a GUI into parmed itself. It's built in some way from python, and doesn't involve any old X-Windows code.

Scott Brozell is looking to possible fixes, i.e. making the pointers acceptable -- this is not "fixing the code" which is not in the cards.

People that need xleap have several options: (a) compile from source code with GCC; (b) use an older version of the conda-forge AmberTools; (c) run it on Linux, either by compiling themselves or using the Linux conda-forge package.

[h-vetinari]

Since ambertools (at least in this feedstock) is being built by CMake, the easiest would be to add -DCMAKE_C_STANDARD=90.

FWIW, here's the section about these changes in the clang 16 release notes.

Scott Brozell is looking to possible fixes, i.e. making the pointers acceptable

Judging from the docs, this is mostly being more judicious about what the types in the function signature are, w.r.t. adding resp. removing * / &.

W.r.t. standardese, I found the following reference:

The rationale for reporting errors is C11 6.5.2.2p9:

"If the function is defined with a type that is not compatible with the type (of the expression) pointed to by the expression that denotes the called function, the behavior is undefined."

[dacase]

the easiest would be to add -DCMAKE_C_STANDARD=90

I tried both this, and forcing the CFLAGS to include -std=c90 or std=c89. Neither had any effect on the errors reported above.

What did have an effect was to add -Wno-error=incompatible-function-pointer-types to CFLAGS. This change allowed xleap to build; (I've not yet tested it). Of course, this just hides the problem, but I'm not too worried here: if the X-window code in the GUI doesn't work correctly, I'm assuming that will be obvious to the user...(!?!)

[dacase]

Relevant change for above is at line 196 of cmake/CompilerFlags.cmake. Change

add_flags(C -Wall -Wno-unused-function)

to

add_flags(C -Wno-error=incompatible-function-pointer-types -Wall -Wno-unused-function)

In the end, if we were to make this change, we'd need to publish an AmberTools update. But maybe someone with access to OSX machines can test this manually(?) It requires an edit after downloading the upstream tarball. Let me know if it would help to make a modified tarball.

[h-vetinari]

If need be we can carry a patch for that locally, no need to republish.

[sbrozell]

The first two errors are complaining that type XtGrabKind, an enum in X11/Intrinsic.h, is not the same type as int. The second two errors are complaining about const-ness, ie, passing a const char to a char . So technically these do flag potentially buggy code, but as a practical matter this is merely routine code for its era.

I'm not sure i know where to put in the syntactic sugar, ie, casts, to make the compiler happy, but i have ideas; however, i did not find an already available clang 16 compiler nor have i tried to install it yet. (To really fix the code to make it bullet proof would likely take lots of study, at least from me.)

[dacase]

Scott: you may not need to do anything -- see comments above about using -Wno-error=incompatible-function-pointer-types

There are pre-built clang compilers here:

https://github.com/llvm/llvm-project/releases/tag/llvmorg-16.0.0

I think "really fixing the code" is not likely a good path to go down.

[sbrozell]

Fixed in the Amber repo, merged into master, will be released with Amber24.

[sbrozell]

BTW, fixing the code was necessary to support oneAPI compilers, so this was a beneficial side effect.