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

Scalar constexpr is odr-used when used as json initializer #1905

Closed
dmarks-ls opened this issue Jan 17, 2020 · 8 comments
Closed

Scalar constexpr is odr-used when used as json initializer #1905

dmarks-ls opened this issue Jan 17, 2020 · 8 comments
Labels
kind: bug solution: invalid the issue is not related to the library state: needs more info the author of the issue needs to provide more details

Comments

@dmarks-ls
Copy link

dmarks-ls commented Jan 17, 2020

I'm not sure if this is a bug or not; hopefully someone more versed in the JSON library or C++ in general can determine that.

I've run into a situation where I'm using a constexpr in a brace initializer for a JSON object, and the linker is complaining that the constexpr is an undefined reference. Here's complete example code:

class Wibble {
 public:
  static constexpr size_t kFoo = 1;
  size_t GetFoo(void);
  size_t GetOne(void);
  static void Test(void);
};

size_t Wibble::GetFoo(void) {
  json document = json { { "foo", kFoo }, };  // line 32
  return document.at("foo").get<size_t>();
}

size_t Wibble::GetOne(void) {
  json document = json { { "one", 1 }, };
  return document.at("one").get<size_t>();
}

void Wibble::Test(void) {
  Wibble wibble;
  printf("foo = %u\n", wibble.GetFoo());
  printf("one = %u\n", wibble.GetOne());
}

/* Comment this line at your peril. */
constexpr size_t Wibble::kFoo;

Test conditions -- I call Wibble::Test() from another module to ensure that all methods are being invoked.

Expected behavior -- If the last line constexpr size_t Wibble::kFoo; is omitted/commented out, the project compiles and links successfully.

Actual behavior -- If the last line shown above is not present, I get a linker error:
my_source_file.cc:32: undefined reference to 'Wibble::kFoo'

My best understanding of what's going on is... I'm expecting that the brace initializer should only need the value of the scalar kFoo that is being supplied. But the compiler is acting like kFoo is "odr-used". That is, it's demanding that there be a static constant "1" in memory identified by the label Wibble::kFoo.

I know that static constexpr items need to be defined and not just declared if they're odr-used (more background here), but I don't see why that's the case here. In Wibble::GetOne(), the value 1 is a satisfactory initializer for the JSON object. kFoo is used in the exact same way in Wibble::GetFoo(), where only its value should be needed, but the compiler is demanding that Wibble::kFoo be explicitly defined.

So, why is using kFoo in the initializer list causing it to be odr-used? Presumably if I referenced some constexpr from some other module, then that constexpr would need to be defined as well, and I shouldn't be defining storage in one module that belongs to a different module... and I shouldn't need to explicitly define each and every constexpr in all of my classes. If kFoo were a complex type, I could understand why it would be need to be odr-used, but it's not, it's a scalar, and only the value of it should be necessary to initialize the object. I'm using C++14, so this shouldn't be an issue for scalars.

So, is this expected behavior?

Details:
Using NXP MCUXpresso IDE for Cortex-M processors
arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 8-2019-q3-update) 8.3.1 20190703 (release) [gcc-8-branch revision 273027]
Using -std=c++14 (ISO C++14)
Using nlohmann/json release 3.7.3

@nlohmann
Copy link
Owner

Can you please provide the full example you are describing? I fail to reproduce the issue with just the class definition above.

@nlohmann nlohmann added the state: needs more info the author of the issue needs to provide more details label Jan 21, 2020
@dmarks-ls
Copy link
Author

This is an embedded project, so my ability to strip back the example is somewhat limited. Here's what I've done: I've put all of the relevant code into a single source code module (json_test.cc), which I've attached here:
json_test.cc.zip

The only two files I #include in the test file are <nlohmann/json.hpp> and <iostream> so there are no dependencies on any of my project-specific headers. I've defined a single C function to invoke the test:

extern "C" void run_json_test(void);

I then include that prototype and invoke that function from somewhere else in my program to make sure it all gets linked. And I still get the linker error.

I've included compiler command line details and the exact linker error message in the source file as well. Hopefully you'll spot something. If it still doesn't trigger for you, I could see about working up a full separate example project in MCUXpresso, so that you could then download MCUXpresso for yourself and try building the project. (Since the error I'm encountering is a linker failure, not a runtime failure, you wouldn't need any embedded hardware in order to reproduce the error.)

Let me know if you're able to get the error to trigger.

@dmarks-ls
Copy link
Author

Fun fact: I've just discovered that regular old assignment triggers this bug for me as well. Example:

size_t Wibble::GetBar(void) {
  json document;
  document["bar"] = kBar;
  return document.at("bar").get<size_t>();
}

I've updated my example code:
json_test_constexpr_odr_used.cc.zip

I thought maybe it would behave differently if it wasn't being used in an initializer list, but apparently not. Hopefully you can reproduce it. If not, I'll work up an MCUXpresso project for you.

@dmarks-ls dmarks-ls changed the title json initializer list causes scalar constexpr to be odr-used Scalar constexpr is odr-used when used as json initializer Jan 29, 2020
@nickaein
Copy link
Contributor

nickaein commented Feb 1, 2020

For easier debugging, I have reproduced the issue here on Godbolt: https://godbolt.org/z/WxmL5Q

The notable differences are:

  • I've selected the GCC x86-64 compiler so the option for binary execution become available
  • Since Godbolt doesn't have the exact JSON library version (3.7.3), I selected trunk version for the library

The behavior seems to be expected though, as pre-C++17 requires defining the static constexpr outside the class (more info). Note that setting -std=c++17 in the Goldbot resolves the issue.

@dmarks-ls
Copy link
Author

Thanks for setting that up @nickaein . I guess I understand the bit about the constructor being defined as taking a const reference, which would demand that the constexpr be instantiated. Presumably operator= is also defined as taking a const reference, which is why the RHS is treated as odr-used.

Is there a way to address this somehow using rvalue references? Or is this a won't-fix?

@nickaein
Copy link
Contributor

nickaein commented Feb 4, 2020

To get back to your question:

My best understanding of what's going on is... I'm expecting that the brace initializer should only need the value of the scalar kFoo that is being supplied.
...
So, why is using kFoo in the initializer list causing it to be odr-used?

That's right, but my guess is that the constructor (initializer_list?) is taking its arguments as const reference. This causes the linker to complain about the kFoo not being defined. Here is a minimal example that demonstrate this behavior: https://godbolt.org/z/FVFL2b

Therefore, any trick that helps to prevent kFoo being passed as const reference can solve the issue, e.g.:

nlohmann::json j{{"key", static_cast<decltype(kFoo)>(kFoo)}};

So, is this expected behavior?

As pointed out in the links above, this is expected (yet not desired) on C++14. This has been "fixed" on C++17 onward.

@nlohmann
Copy link
Owner

Thanks for the analysis @nickaein. I am afraid there is little this library can do, right?

@nickaein
Copy link
Contributor

I agree. Even if we try to detect and handle such cases,

  • It doesn't seem straightforward and could be a brittle feature,

  • As we have to prevent passing by reference for these variables, we possibly degrade the performance even if no link error is going to happen,

  • The user will still get a link error as soon as pass that variable by reference somewhere else.

Nevertheless, this seems a "normal" behavior of pre-C++17 compilers and based on the C++ standard, so we shouldn't try to work around that.

@nlohmann nlohmann added the solution: invalid the issue is not related to the library label Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug solution: invalid the issue is not related to the library state: needs more info the author of the issue needs to provide more details
Projects
None yet
Development

No branches or pull requests

3 participants