|
Notes |
|
|
(0030585)
|
|
Sean McBride
|
|
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
|
|
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
|
|
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
|
|
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
|
|
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
|
|
2013-04-23 10:07
|
|
mmmm, that does seem easier! :)
Want me to do this or are you set up with gerrit? |
|
|
|
(0030599)
|
|
kentwilliams
|
|
2013-04-23 10:24
|
|
|
Go for it. I'm bogged down in other crap. |
|
|
|
(0030602)
|
|
Sean McBride
|
|
2013-04-23 13:11
|
|
|