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

CLD2DynamicDataLoader calls delete instead of delete[] on array types #14

Closed
jariesa opened this issue Jul 27, 2015 · 4 comments
Closed

CLD2DynamicDataLoader calls delete instead of delete[] on array types #14

jariesa opened this issue Jul 27, 2015 · 4 comments

Comments

@jariesa
Copy link
Member

@jariesa jariesa commented Jul 27, 2015

Originally reported on Google Code with ID 14

Upon running some browser tests in Chrome, the following error was encountered when
attempting to call CLD2::loadDataFromRawAddress():

memory allocation/deallocation mismatch at 0x155bb621cb20: allocated with new [] being
deallocated with delete
Received signal 11 SEGV_MAPERR 000000000039
...
#6 0x000002b7b791 MallocBlock::CheckLocked()
#7 0x000002b7b422 MallocBlock::CheckAndClear()
#8 0x000002b7bb4a MallocBlock::Deallocate()
#9 0x000002b79109 DebugDeallocate()
#10 0x000009e02885 operator delete()
#11 0x000006ecd635 CLD2DynamicDataLoader::loadDataInternal()
#12 0x000006ecd325 CLD2DynamicDataLoader::loadDataRaw()
#13 0x000006eba963 CLD2::loadDataFromRawAddress()

I'm not sure why this wasn't caught earlier in testing. It may be a consequence of
toolchain changes in Chromium, but the error seems valid and should be fixed. This
was previously working without issue on both Linux and Android platform builds for
x64 and ARM respectively.

I will review the other uses of delete to see if there are more occurrences. This should
be a trivial fix, but blocks adoption of CLD2 dynamic mode in Chromium.

Reported by andrewhayden@google.com on 2014-05-15 16:32:28

@jariesa

This comment has been minimized.

Copy link
Member Author

@jariesa jariesa commented Jul 27, 2015

$ find . -name "*dynamic*.cc" | xargs grep delete                      
./cld2_dynamic_data_tool.cc:    delete header->tableHeaders;
./cld2_dynamic_data_tool.cc:    delete header;
./cld2_dynamic_data_loader.cc:    delete header;
./cld2_dynamic_data_loader.cc:    delete header;
./cld2_dynamic_data_loader.cc:    delete tableHeaders;
./cld2_dynamic_data_loader.cc:    delete header;
./cld2_dynamic_data_loader.cc:    delete tableHeaders;
./cld2_dynamic_data_loader.cc:  delete((*scoringTables)->unigram_compat_obj); // tableSummaries[0]
from loadDataFile
./cld2_dynamic_data_loader.cc:  delete(*scoringTables);
./cld2_dynamic_data_loader.cc:  delete header->tableHeaders;
./cld2_dynamic_data_loader.cc:  delete header;

I think all we need to fix here is the tableHeaders deletion work.

Reported by andrewhayden@google.com on 2014-05-15 16:37:35

@jariesa

This comment has been minimized.

Copy link
Member Author

@jariesa jariesa commented Jul 27, 2015

This is fixed in r161:
https://code.google.com/p/cld2/source/detail?r=161

I've run all the unit tests again, and confirmed that this fixes the issue in the Chromium
build system as well.

Reported by andrewhayden@google.com on 2014-05-16 10:36:29

  • Status changed: Verified
@jariesa

This comment has been minimized.

Copy link
Member Author

@jariesa jariesa commented Jul 27, 2015

For posterity:

* This doesn't affect the binary file format at all. Previously generated dumps will
still work properly.
* Because the objects pointed to by the pointer were structs instead of class instances,
the use of delete and delete[] should be exactly equivalent (i.e., there is no destructor
to call on each element of the array, so there is nothing gained by adding the [])

This is purely a compiler-happiness change.

Reported by andrewhayden@chromium.org on 2014-05-16 11:45:12

@jariesa

This comment has been minimized.

Copy link
Member Author

@jariesa jariesa commented Jul 27, 2015

Also for posterity, I think what happened is that Chromium's build system finally absorbed
this change:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=29185

Reported by andrewhayden@google.com on 2014-05-16 11:46:17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.