View Issue Details Jump to Notes ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0014013VTK(No Category)public2013-04-16 16:492015-01-09 13:53
Reporterkentwilliams 
Assigned ToSean McBride 
PrioritynormalSeverityminorReproducibilityhave not tried
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target VersionFixed in Version6.0.0 
Summary0014013: Silence alignment change cast in vtkImageReader.cxx
DescriptionRun 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.

TagsNo tags attached.
ProjectTBD
Typeusability
Attached Files

 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 [^]

 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


Copyright © 2000 - 2018 MantisBT Team