Skip to content

Fix rand_r() proper inclusion#7392

Closed
cbalint13 wants to merge 1 commit intoapache:mainfrom
cbalint13:rpc
Closed

Fix rand_r() proper inclusion#7392
cbalint13 wants to merge 1 commit intoapache:mainfrom
cbalint13:rpc

Conversation

@cbalint13
Copy link
Contributor

  • The following error is encountered:
RuntimeError: error while running command "arm-none-eabi-g++ -mcpu=cortex-m33 -std=c++11 -Wall -Werror
...
...
/usr/lib64/python3.9/site-packages/tvm/src/runtime/crt/host/main.cc:106:18: 
error: 'rand_r' was not declared in this scope; did you mean 'random'?
  106 |     int random = rand_r(&random_seed);
      |                  ^~~~~~
      |                  random
  • Now, POSIX rand_r() is not visible using -std=c++11, one have to add -D_GNU_SOURCE or lower the 2011 requirement.

The proposed PR fix the issue, also fixes missing propagation to ldflags (the final binary compilation).

@areusch , @manupa-arm , @mbrookhart please help with the review.

Thank you !

@areusch
Copy link
Contributor

areusch commented Feb 2, 2021

hey @cbalint13 can you give an example code fragment that triggers this bug? i don't think we should be compiling rand_r to bare metal--the main.cc present in src/runtime/crt/host/main.cc is intended to be compiled against a traditional POSIX OS (e.g. is used for simulating a device as a subprocess and treating stdio as the UART link).

@cbalint13
Copy link
Contributor Author

hey @cbalint13 can you give an example code fragment that triggers this bug? i don't think we should be compiling rand_r to bare metal--the main.cc present in src/runtime/crt/host/main.cc is intended to be compiled against a traditional POSIX OS (e.g. is used for simulating a device as a subprocess and treating stdio as the UART link).

@areusch

Can look at this sample: tvm-micro-pr7392.py

@areusch
Copy link
Contributor

areusch commented Feb 2, 2021

@cbalint13 thanks for the example. could you clarify--are you trying to run your demo as:

  1. a simulation with the C runtime executing in a subprocess
  2. on nrf5340dk

I ask because there are a couple of different lines that need to match:

  • line 57 should be either micro('host') or micro(<device>), depending on situation 1 or 2
  • line 76 should either instantiate DefaultCompiler or ZephyrCompiler, depending on situation 1 or 2
  • line 77, the argument should either be (approximately) $CRT_ROOT_DIR/host or path/to/runtime/crt, depending on situation 1 or 2

it seems like based on your comment at the end of the file, you want situation 1--so just fix up line 57 and things should work properly. right now, DefaultCompiler is invoking the ARM cross-compiler because it thinks you are targeting the dev board based on your target definition on line 57.

@areusch
Copy link
Contributor

areusch commented Feb 2, 2021

and to clarify--i'd love to improve the documentation/tutorial, if there are specific parts you found confusing, i'd definitely be interested to learn that.

@cbalint13 cbalint13 changed the title Fix rand_r() rpc proper inclusion Fix rand_r() proper inclusion Feb 2, 2021
@cbalint13
Copy link
Contributor Author

cbalint13 commented Feb 2, 2021

and to clarify--i'd love to improve the documentation/tutorial, if there are specific parts you found confusing, i'd definitely be interested to learn that.

@areusch ,

First thank you for taking your time into the problem !

To not mix up things:

A. we could forbid (+message) if target micro('host') is used with cross/specific ZephyrCompiler & vice versa ?
B. what if someone don't want ZephyrCompiler (just another SDK) but a DefaultCompiler from system instead (like me) ?

By DefaultCompiler i meant my own installed cross arm-none-eabi-g++ in our case (which seems to be invoked at all).
By local/generic vs remote/specific terms i meant micro('host') vs micro(<device>)

So,

  • Should we introduce a CrossCompiler too for such purposes (as a more generic form) ?
  • Then should we forbid local target to generate remote/cross/specific target except CrossCompiler ?

Would like to understand and see your reasoning on the subject first.

@areusch
Copy link
Contributor

areusch commented Feb 2, 2021

@cbalint13 it is a little bit more complicated than just specifying the Compiler implementation when cross-compiling. when using DefaultCompiler, there is assumption that all of the code needed to run the platform (I.e. SoC startup code, peripheral config) lives in a main.cc or in a library you supply to build_static_runtime(extra_libs=). on POSIX systems, this assumption doesn't impose much on the build, but in a bare metal deployment situation it's much different.

to increase compatibility with the many dev boards out there, we compile against a cross-platform deployment framework (Zephyr). most cross-platform bare-metal deployment frameworks include their own build system to determine which sources to include and the compiler config e.g. cflags, ldflags. Zephyr does this from CMakeLists.txt and prj.conf. making it convention to include this config in TVM Python scripts would mean that convention is to replicate the build system from the RTOS you're using.

unfortunately we kind of built out this Compiler abstraction without paying too much attention to that, so it's kind of the wrong level of abstraction to use here. the long-term solution to this problem is to replace Compiler with something more "project-level," where "project" roughly refers to the startup code, RTOS, and other application specific code you need to run models on a bare metal device. See #6 on the µTVM M2 roadmap.

to address your question B: you are still welcome to instantiate a Compiler impl of your choice and this doesn't need to live in the TVM source tree. until the project API refactor is done, happy to include additional impls into the TVM tree so long as they can be tested (see tests/micro/qemu/test_zephyr.py).

in the short term, I think it would be great to change DefaultCompiler to error when you try to compile a file with a cortex-m target. that would be adjusting the logic here. want to take a shot at that?

@manupak
Copy link
Contributor

manupak commented Feb 2, 2021

Hi @areusch ,

A follow up question, what are your thoughts on the effect of the usage of DefaultCompiler.library() function's support to get the operator library as an archive though? I can understand how this affects for DefaultCompiler.binary() or build_static_runtime (that uses DefaultCompiler.binary()). I guess my question is should we block DefaultCompiler.library() for cortex-m targets? -- or should we maybe factor that part out if we are blocking ?

@areusch
Copy link
Contributor

areusch commented Feb 2, 2021

@manupa-arm are you talking about for export_library?

@manupak
Copy link
Contributor

manupak commented Feb 2, 2021

@areusch yes in the case where we just want to use export_library (w/o build_static_runtime) -- as of today we can provide DefaultCompiler.library() as the fcompile if one only wants get operator lib as an .a. I mean I understand there are other ways to do that such as modifying cc.cross_compile to support archive target with using a c compiler

@cbalint13
Copy link
Contributor Author

and to clarify--i'd love to improve the documentation/tutorial, if there are specific parts you found confusing, i'd definitely be interested to learn that.

@areusch

You are right. Also the public docs mentions the different context. Thanks a lot for your time !

Now, to avoid such confusions

  • can we teach TVM to not invoke wrong procedure in wrong context (dropping error messages) ?

Hi @areusch ,

A follow up question, what are your thoughts on the effect of the usage of DefaultCompiler.library() function's support to get the operator library as an archive though? I can understand how this affects for DefaultCompiler.binary() or build_static_runtime (that uses DefaultCompiler.binary()). I guess my question is should we block DefaultCompiler.library() for cortex-m targets? -- or should we maybe factor that part out if we are blocking ?

The blocking shouldnt be done just by cortex-m (targets can be riscv, xtensa and many more). I think of blocking out all != micro(host)

@cbalint13
Copy link
Contributor Author

@areusch yes in the case where we just want to use export_library (w/o build_static_runtime) -- as of today we can provide DefaultCompiler.library() as the fcompile if one only wants get operator lib as an .a. I mean I understand there are other ways to do that such as modifying cc.cross_compile to support archive target with using a c compiler

Very good idea. But not sure that the micro backend should do this. But if you look at my gist example there already a save .o object file (and is not done by micro backend !).

@cbalint13
Copy link
Contributor Author

cbalint13 commented Feb 2, 2021

in the short term, I think it would be great to change DefaultCompiler to error when you try to compile a file with a cortex-m target. that would be adjusting the logic here. want to take a shot at that?

@areusch ,

I will propose a protection way to avoid missusage in a separate PR.

I am closing the PR here, @manupa-arm , @areusch Lets continue on the discuss forum (right on rfc)

Thanks a lot !

@cbalint13 cbalint13 closed this Feb 2, 2021
@areusch
Copy link
Contributor

areusch commented Feb 2, 2021

thanks @cbalint13 @manupa-arm for the great discussions. I will post up an RFC to discuss project generator API in the coming week. feel free to open topics as well if you'd like to discuss things.

@cbalint13 cbalint13 deleted the rpc branch May 28, 2022 07:09
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.

3 participants