Command line option processing fails on ARM architectures

[merkys]

I am building and testing FreeSASA 2.1.1 on all architectures supported by Debian (full build matrix). For most of them build and test passes just fine, except ARM architectures (arm64, armel, armhf) where most of the tests fail due to command line options:

FAIL: test-cli
==============

== Basic file errors ==

== General options ==
freesasa: error: unknown option '1'

Call 'freesasa -h' or 'man freesasa' for usage instructions

Report bugs to <https://github.com/mittinatten/freesasa/issues>
Home page: <http://freesasa.github.io>

Error: "../src/freesasa -o tmp/test-cli.dump -e tmp/err -n 1 ../tests/data/rsa/GLY.pdb" failed

I am compiling with GCC/G++ 11.3.0 and without -lc++ in freesasa_LDADD.

[mittinatten]

Thanks for reporting. This looks strange, as fas as I can tell there is something wrong with how getopt parses options. I don't have access to a an ARM machine, and can't really debug this, unfortunately. Neither could I find any reports online that the library has issues with ARM.

Just to rule out anything funky with the test-runner, could you verify that you get the same error if you run the same/similar command manually?

[merkys]

Thanks for prompt reply.

Just to rule out anything funky with the test-runner, could you verify that you get the same error if you run the same/similar command manually?

Yes, the same freesasa: error: unknown option '1' appears when I attempt calling src/freesasa -n 1 tests/data/rsa/GLY.pdb from the command line.

[mittinatten]

Have you been able to build older versions of the library, < 2.1, for instance, which would be pure C?

Cool btw that you are packaging this for Debian. I thought about it myself a couple of years ago, and added a few things (man-pages at least) to be compliant, but never got around to actually doing it.

[merkys]

Have you been able to build older versions of the library, < 2.1, for instance, which would be pure C?

I can confirm the same issue exists in 2.0.3 as well.

Cool btw that you are packaging this for Debian. I thought about it myself a couple of years ago, and added a few things (man-pages at least) to be compliant, but never got around to actually doing it.

I believe FreeSASA is a nice addition for Debian. Existence of manpages indeed made the packaging easier!

[mittinatten]

Does it look like it's always -n that causes the problem? Do you get the same error if you use the long form --resolution 1?

Since I don't have any ARM machine available, I can't really help you much, but this is where the n-option is handled in the code: https://github.com/mittinatten/freesasa/blob/master/src/main.cc#L610, and this is where the option is defined: https://github.com/mittinatten/freesasa/blob/master/src/main.cc#L53 and https://github.com/mittinatten/freesasa/blob/master/src/main.cc#L85

There's nothing special about -n as far as I can tell.

[merkys]

OK, it seems I have located the issue and found a fix. The problem is with the following code:

static int
parse_arg(int argc, char **argv, struct cli_state *state)
{
    int alg_set = 0;
    char opt;
    int n_opt = 'z' + 1;
    char opt_set['z' + 1];
    int option_index = 0;
    FILE *cf;

    memset(opt_set, 0, n_opt);

    while ((opt = getopt_long(argc, argv, options_string,
                              long_options, &option_index)) != -1) {

It seems that char is unsigned by default on ARM. Thus when the return value of -1 from getopt_long is cast into an unsigned char, it will be 255, and the loop never stops.

My suggestion is to retype char opt as int opt. This way -1 will stay -1 no matter the specifics of char. I have tested this change and it worked both on amd64 and arm64.

[mittinatten]

Thanks! That was a simple solution. Do you want to submit a PR? Otherwise I will implement the change soonish.

The CI setup we have has stopped working, so I'm on the lookout for something new (#80), I'll keep testing on ARM in mind.

[merkys]

I will open a PR soon. I have tested the solution on all Debian architectures and it seems to work everywhere now.

[mittinatten]

Thank you for the PR. I will release 2.1.2 with the fix shortly. Looks like from the build logs that you configured without --enable-check which allows running a much more thorough test suite.

The CLI tests test almost all important functionality but the other tests cover a bunch of edge cases that can't be checked via the CLI directly. The tests however involve mocking errors from central functions like malloc and strdup so it might not behave on all platforms.

[mittinatten]

This is now available as Freesasa 2.1.2

[merkys]

Thank you for the PR. I will release 2.1.2 with the fix shortly. Looks like from the build logs that you configured without --enable-check which allows running a much more thorough test suite.

The CLI tests test almost all important functionality but the other tests cover a bunch of edge cases that can't be checked via the CLI directly. The tests however involve mocking errors from central functions like malloc and strdup so it might not behave on all platforms.

Thanks for accepting the PR! Good point about --enable-check, I was not aware of it, will enable soon and let you know should anything fail.