View Issue Details [ Jump to Notes ] | [ Print ] | ||||||||
ID | Project | Category | View Status | Date Submitted | Last Update | ||||
0014013 | VTK | (No Category) | public | 2013-04-16 16:49 | 2015-01-09 13:53 | ||||
Reporter | kentwilliams | ||||||||
Assigned To | Sean McBride | ||||||||
Priority | normal | Severity | minor | Reproducibility | have not tried | ||||
Status | closed | Resolution | fixed | ||||||
Platform | OS | OS Version | |||||||
Product Version | |||||||||
Target Version | Fixed in Version | 6.0.0 | |||||||
Summary | 0014013: Silence alignment change cast in vtkImageReader.cxx | ||||||||
Description | Run into this with our 'paranoid' build flags and CLang. Git version 86d156256c76a98 CLang complains about line 324 in vtkImageReader.cxx Not a real problem because the templating should mean that casting to a wider type is actually a good cast. The patch below makes the warning go away, though there might be a more elegant solution: diff --git a/IO/Image/vtkImageReader.cxx b/IO/Image/vtkImageReader.cxx index 1125fee..eaccc09 100644 --- a/IO/Image/vtkImageReader.cxx +++ b/IO/Image/vtkImageReader.cxx @@ -321,7 +321,7 @@ void vtkImageReaderUpdate2(vtkImageReader *self, vtkImageData *data, } // copy the bytes into the typed data - inPtr = (IT *)(buf); + inPtr = (IT *)(void *)(buf); for (idx0 = dataExtent[0]; idx0 <= dataExtent[1]; ++idx0) { // Copy pixel into the output. | ||||||||
Tags | No tags attached. | ||||||||
Project | TBD | ||||||||
Type | usability | ||||||||
Attached Files | |||||||||
Relationships | |
Relationships |
Notes | |
(0030585) Sean McBride (developer) 2013-04-22 11:31 |
I recently have been enabling many clang warnings and fixing as many as I could. I saw this one but didn't fix it because I'm not convinced it is a false positive.... 'buf' is allocated using "new unsigned char" regardless of the templated type, so it seems to me that it could indeed be only byte aligned. |
(0030589) kentwilliams (reporter) 2013-04-22 12:27 |
As usually happens when I bump up against C++ standard issues, I'm confused by what I read on the Internet about this topic. But are two possibilities: 1. builtin new will always return a memory space that's aligned for the largest POD type. This seems to be implied by several people's commentary on the C++ standards. 2. builtin new only aligns returned memory to native alignment requirements for the type allocated. If #1 is the case, then there's no problem with this patch. If #2 is the case, something like the following patch would force alignment. Of course if #2 is the case, I wonder why the existing code hasn't caused a problem. diff --git a/IO/vtkImageReader.cxx b/IO/vtkImageReader.cxx index fc7627e..75e58d0 100644 --- a/IO/vtkImageReader.cxx +++ b/IO/vtkImageReader.cxx @@ -258,7 +258,7 @@ void vtkImageReaderUpdate2(vtkImageReader *self, vtkImageData *data, // create a buffer to hold a row of the data - buf = new unsigned char[streamRead]; + buf = reinterpret_cast<unsigned char *>(new long long[(streamRead / sizeof(long long)) + 1]); target = (unsigned long)((dataExtent[5]-dataExtent[4]+1)* (dataExtent[3]-dataExtent[2]+1)/50.0); @@ -269,7 +269,7 @@ void vtkImageReaderUpdate2(vtkImageReader *self, vtkImageData *data, { if ( !self->OpenAndSeekFile(dataExtent,0) ) { - delete [] buf; + delete [] reinterpret_cast<long long *>(buf); return; } } @@ -279,7 +279,7 @@ void vtkImageReaderUpdate2(vtkImageReader *self, vtkImageData *data, { if ( !self->OpenAndSeekFile(dataExtent,idx2) ) { - delete [] buf; + delete [] reinterpret_cast<long long *>(buf); return; } } @@ -310,7 +310,7 @@ void vtkImageReaderUpdate2(vtkImageReader *self, vtkImageData *data, << ", Skip0 = " << streamSkip0 << ", Skip1 = " << streamSkip1 << ", FilePos = " << static_cast<vtkIdType>(self->GetFile()->tellg())); - delete [] buf; + delete [] reinterpret_cast<long long *>(buf); return; } // handle swapping @@ -321,7 +321,7 @@ void vtkImageReaderUpdate2(vtkImageReader *self, vtkImageData *data, } // copy the bytes into the typed data - inPtr = (IT *)(buf); + inPtr = (IT *)static_cast<void *>(buf); for (idx0 = dataExtent[0]; idx0 <= dataExtent[1]; ++idx0) { // Copy pixel into the output. @@ -390,7 +390,7 @@ void vtkImageReaderUpdate2(vtkImageReader *self, vtkImageData *data, } // delete the temporary buffer - delete [] buf; + delete [] reinterpret_cast<long long *>(buf); } |
(0030590) Sean McBride (developer) 2013-04-22 12:46 |
I'm no language lawyer, but my understanding is that it's case 2. In practice, most compilers'/libraries' 'new' implementation probably give a generous alignment because there is a lot of code like this out there, but they aren't obliged by the standard to align so generously. As you say, something like your second patch could work, but it's a big ugly.... Another option might be to use malloc(), which I believe guarantees a generous alignment. Just curious here: are you trying to get VTK -Wcast-align clean? As I said, I tried that already, and fixed the easy ones, but the remaining ones are harder. :) |
(0030595) Sean McBride (developer) 2013-04-22 18:19 |
If we could require C++11 we could use "alignas(type)"... I just tried a simple example: --------------------- int main(void) { unsigned char* buf = new unsigned char[54912]; long* buf2 = (long*)buf; // warns! long* buf3 = reinterpret_cast<long*>(buf); // no warning return 0; } --------------------- and get: --------------------- test.cxx:5:16: warning: cast from 'unsigned char *' to 'long *' increases required alignment from 1 to 8 [-Wcast-align] long* buf2 = (long*)buf; // warns! ^~~~~~~~~~ --------------------- So it seems clang warns if it's a C-style cast, but not for a reinterpret_cast. I find that odd... you? |
(0030597) kentwilliams (reporter) 2013-04-23 09:50 |
Actually we've been thinking way too hard on this one. I didn't follow the computation of 'streamRead' -- but that could be simplified too. Why not just allocate an array of 'IT' instead an array of char? diff --git a/IO/vtkImageReader.cxx b/IO/vtkImageReader.cxx index fc7627e..6e213ba 100644 --- a/IO/vtkImageReader.cxx +++ b/IO/vtkImageReader.cxx @@ -203,7 +203,7 @@ void vtkImageReaderUpdate2(vtkImageReader *self, vtkImageData *data, long streamSkip0, streamSkip1; unsigned long streamRead; int idx0, idx1, idx2, pixelRead; - unsigned char *buf; + IT *buf; int inExtent[6]; int dataExtent[6]; int comp, pixelSkip; @@ -258,7 +258,7 @@ void vtkImageReaderUpdate2(vtkImageReader *self, vtkImageData *data, // create a buffer to hold a row of the data - buf = new unsigned char[streamRead]; + buf = new IT[streamRead/sizeof(IT)]; target = (unsigned long)((dataExtent[5]-dataExtent[4]+1)* (dataExtent[3]-dataExtent[2]+1)/50.0); |
(0030598) Sean McBride (developer) 2013-04-23 10:07 |
mmmm, that does seem easier! :) Want me to do this or are you set up with gerrit? |
(0030599) kentwilliams (reporter) 2013-04-23 10:24 |
Go for it. I'm bogged down in other crap. |
(0030602) Sean McBride (developer) 2013-04-23 13:11 |
http://review.source.kitware.com/#/t/2671 [^] |
Notes |
Issue History | |||
Date Modified | Username | Field | Change |
2013-04-16 16:49 | kentwilliams | New Issue | |
2013-04-22 11:31 | Sean McBride | Note Added: 0030585 | |
2013-04-22 11:37 | Sean McBride | Assigned To | => Sean McBride |
2013-04-22 11:37 | Sean McBride | Status | backlog => tabled |
2013-04-22 12:27 | kentwilliams | Note Added: 0030589 | |
2013-04-22 12:46 | Sean McBride | Note Added: 0030590 | |
2013-04-22 18:19 | Sean McBride | Note Added: 0030595 | |
2013-04-23 09:50 | kentwilliams | Note Added: 0030597 | |
2013-04-23 10:07 | Sean McBride | Note Added: 0030598 | |
2013-04-23 10:24 | kentwilliams | Note Added: 0030599 | |
2013-04-23 13:11 | Sean McBride | Note Added: 0030602 | |
2013-04-23 17:07 | Sean McBride | Status | tabled => customer review |
2013-04-23 17:07 | Sean McBride | Resolution | open => fixed |
2013-07-22 19:34 | Dave DeMarle | Status | customer review => closed |
2013-07-22 19:34 | Dave DeMarle | Fixed in Version | => 6.0.0 |
2015-01-09 13:53 | Sean McBride | Source_changeset_attached | => VTK master 57745b3b |
Issue History |
Copyright © 2000 - 2018 MantisBT Team |