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

Compile warning on architectures that are not x86 #1939

Closed
tasleson opened this issue Feb 12, 2020 · 7 comments · Fixed by #2144
Closed

Compile warning on architectures that are not x86 #1939

tasleson opened this issue Feb 12, 2020 · 7 comments · Fixed by #2144
Assignees
Labels
kind: bug release item: 🔨 further change solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@tasleson
Copy link

tasleson commented Feb 12, 2020

  • What is the issue you have?

Compiling library results in a compiler error warning which using the latest compilers on non-x86 architectures using the gcc compiler included in fedora 32 aka. rawhide.

  • Please describe the steps to reproduce the issue. Can you provide a small but working code example?

Simply try to compile the library.

  • What is the expected behavior?

Compile success

  • And what is the actual behavior instead?

Compile error warning

./json.hpp:22700:42:   required from here
./json.hpp:8494:24: error: comparison is always true due to limited range of data type [-Werror=type-limits]
 8494 |             if ('\x00' <= c and c <= '\x1F')
      |                 ~~~~~~~^~~~

Not listed on the supported compilers. Fedora rawhide, which I believe is using GCC 10.0.1.

  • Did you use a released version of the library or the version from the develop branch?

Used latest release.

I can for x86, but not easily for other architectures at the moment.

@nlohmann
Copy link
Owner

The example is a warning that only becomes in array with -Werror. Does the code compile if you do not treat the warning as error?

@nlohmann
Copy link
Owner

(I am also not sure all non-x86 machines have this error, because we can compile successfully on ARM, see https://github.com/nlohmann/json#supported-compilers)

@tasleson
Copy link
Author

The example is a warning that only becomes in array with -Werror. Does the code compile if you do not treat the warning as error?

Yes it should, sorry should have been explicit about that detail. The project we are using the library on has this enabled by default and I forgot about it.

(I am also not sure all non-x86 machines have this error, because we can compile successfully on ARM, see https://github.com/nlohmann/json#supported-compilers)

Older versions of the g++ compiler do indeed compile fine for all architectures we compile for. We have been compiling an older version of the library on quite a few different architectures for the past couple of years. I updated to the latest json release and re-ran before submitting this issue. I believe this is a new check added with the most recent g++ which is now flagging this.

IMHO it would be better to be explicit if characters should be signed or unsigned and make changes to accommodate whatever decision is chosen instead of leaving it up as an implementation detail of the tooling.

@nlohmann
Copy link
Owner

Thanks for checking back. I'll have a look at #1940.

@nlohmann
Copy link
Owner

I tried to reproduce the problem locally and compiled the library successfully with

  • -funsigned-char -Werror=type-limits
  • -fsigned-char -Werror=type-limits

I got no error in either setting. Any ideas?

@tasleson
Copy link
Author

Here is the actual builds that failed: https://koji.fedoraproject.org/koji/taskinfo?taskID=41468709

Details of the command line option for the failure, snippet from here

 g++ -std=c++11 -DHAVE_CONFIG_H -I. -I.. -I../c_binding/include -I../c_binding/include -I./c_binding/include -I/usr/include/libxml2 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -Wall -Werror -Wextra -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -march=armv7-a -mfpu=vfpv3-d16 -mtune=generic-armv7-a -mabi=aapcs-linux -mfloat-abi=hard -c lsm_datatypes.cpp  -fPIC -DPIC -o .libs/lsm_datatypes.o

I think to get this to re-create may require gcc 10.0.1 which I don't think has been released yet.

@tasleson tasleson changed the title Failure to compile on architectures that are not x86 Compile warning on architectures that are not x86 Feb 18, 2020
@stale
Copy link

stale bot commented Mar 19, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Mar 19, 2020
@stale stale bot closed this as completed Mar 26, 2020
@nlohmann nlohmann reopened this May 28, 2020
@stale stale bot removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label May 28, 2020
@nlohmann nlohmann linked a pull request May 28, 2020 that will close this issue
@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label May 28, 2020
@nlohmann nlohmann added this to the Release 3.8.0 milestone May 28, 2020
@nlohmann nlohmann self-assigned this May 28, 2020
algitbot pushed a commit to alpinelinux/aports that referenced this issue Jan 18, 2021
Due to `-Werror`, this fails to build on gcc >= 10.0.1. This has been
[reported][0] to json upstream, and fixed in this [pr][1].

Backport the fix to horizon

[0]:nlohmann/json#1939
[1]:nlohmann/json#2144
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug release item: 🔨 further change solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants