Closed GoogleCodeExporter closed 8 years ago
GoogleCodeExporter
commented
8 years ago i'm curious: why this field in particular?
Does it happen with this cost_ field when re-ordering? Or is it triggered by
another field then, once reordered?
Original comment by pascal.m...@gmail.com on 13 Sep 2015 at 1:33
GoogleCodeExporter
commented
8 years ago Turns out I overlooked the fact that VP8LHistogram isn't packed - the members
in there are always aligned "suitably" no matter where in the struct they are
defined. Things change of course if I #PRAGMA pack the whole thing, but that
doesn't change the BUS errors. They are always triggered at the first line of
code that touches a double in a histogram that is incorrectly aligned.
Depending on the input image, it's either the location above or (line number
should be 398)
[UpdateHistogramCost +0x4,0x439440]
h->red_cost_ = PopulationCost(h->red_, NUM_LITERAL_CODES);
> 0 UpdateHistogramCost()
1 HistogramCopyAndAnalyze()
2 VP8LGetHistoImageSymbols()
3 EncodeImageInternal()
4 VP8LEncodeStream()
The actual culprit is VP8AllocateHistogramSet(). While malloc() always returns
a pointer that is aligned % 8 (as is necessary for the double ...cost_),
memory* is aligned only % 4 when the histogram[i] are initialized if size is
even and pointers are 32 bit.
For example, when saving 5.webp (the fire breathing man from the sample
gallery), the patch below leads to the output below:
initmem: @1028f028
mem: @1028f03c
realign: @1028f040
h: 0 @1028f040
h: 1 @10290178
initmem: @1028f028
mem: @1028f038
h: 0 @1028f038
initmem: @1028f028
mem: @1028f038
h: 0 @1028f038
initmem: @116ddfc8
mem: @116debd4
realign: @116debd8
h: 0 @116debd8
h: 1 @116dfd10
h: 2 @116e0e48
h: 3 @116e1f80
h: 4 @116e30b8
h: 5 @116e41f0
h: 6 @116e5328
h: 7 @116e6460
h: 8 @116e7598
h: 9 @116e86d0
[...]
As an alternative to the uintptr_t arithmetic, one can just make sure that
there's an even number of pointers before the histograms (and +8 for the size
could be reduced to +sizeof(*set)):
if (size % 2 == 0) {
memory += sizeof(*set);
Maybe add && (sizeof(*set) != sizeof(double)).
The patch below fixes all SIGBUS errors for me, I haven't managed to cause any
even after saving dozens of random images with xv and ~300 more with cwebp
-lossless. No memcpy required.
--- src/libwebp-0.4.3/src/enc/histogram.c Wed Mar 11 07:06:09 CET 2015
+++ histogram_debug.c Mon Sep 14 23:46:19 CEST 2015
@@ -103,9 +103,11 @@
VP8LHistogramSet* set;
const size_t total_size = sizeof(*set)
+ sizeof(*set->histograms) * size
- + (size_t)VP8LGetHistogramSize(cache_bits) * size;
+ + (size_t)VP8LGetHistogramSize(cache_bits) * size
+ + 8;
uint8_t* memory = (uint8_t*)WebPSafeMalloc(total_size, sizeof(*memory));
if (memory == NULL) return NULL;
+fprintf(stderr, "initmem: @%x\n", memory);
set = (VP8LHistogramSet*)memory;
memory += sizeof(*set);
@@ -113,8 +115,20 @@
memory += size * sizeof(*set->histograms);
set->max_size = size;
set->size = size;
+
+#ifdef WEBP_FORCE_ALIGNED
+fprintf(stderr, "mem: @%x\n", memory);
+if ((uintptr_t)(memory) % 8 != 0) {
+ memory += 8 - ((uintptr_t)memory%8);
+fprintf(stderr, "realign: @%x\n", memory);
+}
+#endif
+
for (i = 0; i < size; ++i) {
set->histograms[i] = (VP8LHistogram*)memory;
+#ifdef WEBP_FORCE_ALIGNED
+fprintf(stderr, "h: %i @%x\n", i, set->histograms[i]);
+#endif
// literal_ won't necessary be aligned.
set->histograms[i]->literal_ = (uint32_t*)(memory + sizeof(VP8LHistogram));
VP8LHistogramInit(set->histograms[i], cache_bits);
Original comment by rainer.c...@sevenval.com on 14 Sep 2015 at 11:09
GoogleCodeExporter
commented
8 years ago thanks for the analysis!
There's indeed a need for some cleanup on the packed malloc's, to add some
proper ALIGN.
Patch to follow...Original comment by s...@google.com on 16 Sep 2015 at 6:51
GoogleCodeExporter
commented
8 years ago https://chromium-review.googlesource.com/#/c/300289/ has been merged:
cd82440 VP8LAllocateHistogramSet: align histogram[] entries
it should address the issue similarly to your patch. let us know if there's any
further breakage and thanks again for the report.Original comment by jz...@google.com on 18 Sep 2015 at 6:22
GoogleCodeExporter
commented
8 years ago The patch in cd82440 on top of libwebp 0.4.3 works for me on IRIX.Original comment by rainer.c...@sevenval.com on 18 Sep 2015 at 11:08
GoogleCodeExporter
commented
8 years ago Great, thanks for trying it out. I'll pull this into the 0.4.4 release once I
branch for that.Original comment by jz...@google.com on 19 Sep 2015 at 1:20
GoogleCodeExporter
commented
8 years ago [closing as fixed]Original comment by pascal.m...@gmail.com on 10 Oct 2015 at 6:06
Original issue reported on code.google.com by
rainer.c...@sevenval.comon 10 Sep 2015 at 11:11