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

Replace type punning with memcpy. #7415

Merged
merged 1 commit into from
Feb 11, 2021

Conversation

grant-arm
Copy link
Contributor

@grant-arm grant-arm commented Feb 5, 2021

The type punning in the existing code is undefined behaviour in C.
In particular, the existing code fails when running on Arm Cortex-M devices.
On Cortex-M, accessing a uint64_t that is not 8-byte aligned generates a hard fault.

The GCC bugs page (https://gcc.gnu.org/bugs/) Non-Bugs/C/Casting section provides a good explanation of why "Dereferencing a pointer that violates the aliasing rules results in undefined behavior."

@manupa-arm @areusch @liangfu

The type punning in the existing code is undefined behaviour in C.
In particular, the existing code fails when running on Arm Cortex-M devices.
On Cortex-M, accessing a uint64_t that is not 8-byte aligned generates a hard fault.

Change-Id: I2aecaa220e581af7c91a8bc7886499d70e2aa6f2
@areusch
Copy link
Contributor

areusch commented Feb 5, 2021

@grant-arm this looks great! want to add a unit test in tests/crt?

@liangfu
Copy link
Member

liangfu commented Feb 6, 2021

@grant-arm this looks great! want to add a unit test in tests/crt?

@areusch can you explain a bit more on how to test the changes brought in this PR?

@areusch
Copy link
Contributor

areusch commented Feb 6, 2021

@liangfu @grant-arm it seems like we should be able to call TVMNDArray_Load() with strm non-aligned. Specifically, malloc a buffer for strm that's WORD_SIZE -1 larger, then intentionally choose a starting point for strm that isn't word-aligned.

@manupak
Copy link
Contributor

manupak commented Feb 7, 2021

@areusch I think we can still call TVMNDArray_Load() with strm non-aligned. Its just that when de-referencing a pointer to non word-aligned location -- we should do it under memcpy so that it does what required based on the target architecture. Inside TVMNDArray_Load, I think data is already accessed via memcpy and its just some metadata such as shape are the ones directly de-referenced. Was that your concern ?

I think this might trigger a compiler warning (-Wcast-align) and a runtime error in some platforms. Therefore, depending on whether this fix clears away all cast-align warnings, we might be able to use -Wcast-align=strict which makes the warning appear for x86 too, if we are using gcc 8 or higher. If this is the case, we might be able to create a failing compilation test case using build_static_runtime ?

@areusch
Copy link
Contributor

areusch commented Feb 8, 2021

@manupa-arm @grant-arm it seems to me that this change is replacing pointer dereferences to **strm with memcpy. this is the correct thing to do, so now we just need to test it. the issue now is that it compiles fine, but at runtime would trigger an unaligned load. this issue should be present across platforms. I think we could write a test that intentionally calls TVMNDArray_Load with unaligned strm. i'm not suggesting we need to test every single case of mis-alignment, but a single test that calls TVMNDArray_Load with unaligned strm should be sufficient to trigger the failure without this patch.

now as to producing unaligned strm: I'm suggesting we malloc a block larger than we need, add n bytes to the pointer (n = 7 - ((uintptr_t) ptr) & 0x7), and fill that location with valid strm. then pass to TVMNDArray_Load.

@grant-arm
Copy link
Contributor Author

Thank you for the feedback @areusch .

I'm going to take a look at implementing a unit test along the lines of your suggestion.

@manupak
Copy link
Contributor

manupak commented Feb 8, 2021

@areusch @grant-arm, what I proposed was some trick to make it also a compiler failure using -Werror with -Wcast-align=strict. This way the compiler will not allow such code to be compiled in the first place. What do you think ?

@areusch
Copy link
Contributor

areusch commented Feb 8, 2021

@manupa-arm i'm not super-familiar with -Wcast-align=strict and its availability cross-platform. we could try adding it to the CFLAGS and see, though...I guess that should catch cases like this. i'm not sure if we have other cases where we have assumed this but it is in practice fine, though.

@u99127
Copy link
Contributor

u99127 commented Feb 11, 2021

@manupa-arm @grant-arm it seems to me that this change is replacing pointer dereferences to **strm with memcpy. this is the correct thing to do, so now we just need to test it. the issue now is that it compiles fine, but at runtime would trigger an unaligned load. this issue should be present across platforms. I think we could write a test that intentionally calls TVMNDArray_Load with unaligned strm. i'm not suggesting we need to test every single case of mis-alignment, but a single test that calls TVMNDArray_Load with unaligned strm should be sufficient to trigger the failure without this patch.

Sorry to jump in but I think a couple of points need to be considered here.
You need an architecture that traps on unaligned accesses. AFAIK, that's not x86_64 in these cases. And thus it is unlikely that you will provoke this AFAIK. this is one of the common cases where things go wrong in this case. AFAIK it will not trap on x86_64. This is a common problem when undefined C is ported between x86_64 and architectures that have strict alignment. My understanding is that you will not see this failure.

I will point out that once we start running tests on Corstone 300 (Cortex-M + Ethos-U), note that the runtime will run on the Cortex-M and publish that upstream in a month or so, there will be a test on a target in the CI that actually breaks for this. After all if the runtime fails with misaligned faults it's fairly fundamental to running any tests.

-Wcast-align=strict works well across all architectures as an option from GCC-8 ( notably it wouldn't work on x86_64 even if the variant of -Wcast-align=strict was setup in GCC 7 as IIRC x86_64 is not a STRICT_ALIGNMENT target in GCC parlance). The default compiler in CI is from Ubuntu 18.04 and thus will not show up on x86_64 in GCC-7 .

$> gcc -Wcast-align=strict /tmp/tst.c
gcc: error: unrecognized command line option ‘-Wcast-align=strict’; did you mean ‘-Wcast-align’?
$> gcc -v
gcc version 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04)

I would suggest fixing the issue, it's undefined C .

Yes it's nice to have a test but that will come automatically with testing initial support for Ethos-U55 in the Corstone300 FVP or any Cortex-M test that runs on an FVP.

Thanks,
Ramana

Copy link
Contributor

@u99127 u99127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@u99127 @grant-arm okay sorry to delay this one--for some reason I was under the impression x86_64 raised SIGBUS on unaligned accesses. looks like i'm dead wrong on that. i agree given that's false, a unit test would be hard to write, LGTM. thanks for fixing this!

@areusch
Copy link
Contributor

areusch commented Feb 11, 2021

@tmoreau89 want to merge?

Copy link
Contributor

@tmoreau89 tmoreau89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tmoreau89 tmoreau89 merged commit c52c176 into apache:main Feb 11, 2021
Lokiiiiii pushed a commit to Lokiiiiii/tvm that referenced this pull request Mar 2, 2021
The type punning in the existing code is undefined behaviour in C.
In particular, the existing code fails when running on Arm Cortex-M devices.
On Cortex-M, accessing a uint64_t that is not 8-byte aligned generates a hard fault.

Change-Id: I2aecaa220e581af7c91a8bc7886499d70e2aa6f2
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Mar 2, 2021
The type punning in the existing code is undefined behaviour in C.
In particular, the existing code fails when running on Arm Cortex-M devices.
On Cortex-M, accessing a uint64_t that is not 8-byte aligned generates a hard fault.

Change-Id: I2aecaa220e581af7c91a8bc7886499d70e2aa6f2
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.

6 participants