Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix wrong behaviour of torad_coord() with gcc 8.1 -O2 (fixes #1084) #1087

Merged
merged 1 commit into from
Aug 11, 2018

Conversation

rouault
Copy link
Member

@rouault rouault commented Aug 10, 2018

torad_coord() of gie.c has this sequence:

if( cond )
    axis = l->param + strlen ("axis=");
n = strlen (axis);

When the if branch is evaluated, n is always zero
even if on inspection axis is non empty

The reason is that the struct ARG_list which is the
type of l use a variable-length array for the param member

struct ARG_list {
paralist *next;
char used;
char param[1];
};

But this is not a proper way of declaring it, and
gcc 8 has apparently optimizations to detect that l->param + 5
points out of the array, hence it optimizes strlen() to 0.

According to https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html,
the proper way of declaring such arrays is to use [0]

@rouault
Copy link
Member Author

rouault commented Aug 10, 2018

looking closer, looks like a too aggressive optimization in that particular case. Filed as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86914

@kbevers
Copy link
Member

kbevers commented Aug 11, 2018

Do you still want to merge this pull request or just wait until the GCC guys sort their things out?

@rouault
Copy link
Member Author

rouault commented Aug 11, 2018

I think that it is reasonable to merge it, even if it is a GCC bug. This version is out and so even if it is fixed, there's always the risk someone sticking with this version and hitting the issue. After all the use of one-element array was discouraged by the gcc docs, so the use of zero-length is a better practice.
(an even better practice would be to just avoid the use of such tricky features)

@kbevers
Copy link
Member

kbevers commented Aug 11, 2018

Alright, let's merge it then. Perhaps put in a comment explaining what's going on.

After 5.2 is released we can finally begin to make breaking changes in projects.h. We should use that opportunity to deal with this in a smarter way that doesn't require hacks for different compilers.

torad_coord() of gie.c has this sequence:
```
if( cond )
    axis = l->param + strlen ("axis=");
n = strlen (axis);
```

When the if branch is evaluated, n is always zero
even if on inspection axis is non empty

The reason is that the struct ARG_list which is the
type of l use a variable-length array for the param member

struct ARG_list {
    paralist *next;
    char used;
    char param[1];
};

But this is not a proper way of declaring it, and
gcc 8 has apparently optimizations to detect that l->param + 5
points out of the array, hence it optimizes strlen() to 0.

Reported as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86914

According to https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html,
the proper way of declaring such arrays is to use [0]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants