Copy or assignment weirdness in StringArray?


#1

I don’t have the time to fully track this down since I have a workaround, but I think I’ve found a feature deficiency in StringArray that results in uninitialized pointers occurring in what should be vanilla code.

[code]StringArray getFileExtensions() {
const tchar* const extensions[] = {
JUCE_T(".mp3"), JUCE_T(".mp2"), JUCE_T(".mp1"), NULL
};
return StringArray(extensions);
}

const StringArray& getFileExtensionsWorkaround() {
static StringArray fileExtensions = getFileExtensions();
return fileExtensions;
}[/code] then later[code]class Format : public AudioFormat {
public:
Format() : AudioFormat(getTranslatedName(), getFileExtensions()) {}
// …
};

void test() {
Format format;
}[/code]
Trying to instantiate Format results in an EXC_BAD_ACCESS signal (full stack trace given below) and when I dig down I see that the StringArray is pointing off into the clouds…

BUT if I simply replace the call to getFileExtensions() with getFileExtensionsWorkaround() then it all works - even though getFileExtensionsWorkaround() called getFileExtensions()…

I have a unit test that exhibits this, and it will be up on http://ax.to/swirlyjuce fairly shortly.

if you just used STL you wouldn’t have to keep reinventing the wheel - and fixing it when it fell off! :stuck_out_tongue:

Actually, you should be using begin/end pairs to pass around lists of things, as those are “format agnostic” and work with pretty well any framework I’ve ever used in C++…

#0 0x0000aa66 in juce::Atomic::operator++ (this=0xfffffff4) at juce_Atomic.h:274
#1 0x001df08d in juce::StringHolder::retain (text=0x0) at /Users/tom/Documents/development/juce/Builds/MacOSX/…/…/src/text/juce_String.cpp:91
#2 0x001d9c1f in juce::String::String (this=0x4140000, other=@0x4140000) at /Users/tom/Documents/development/juce/Builds/MacOSX/…/…/src/text/juce_String.cpp:202
#3 0x001e258d in juce::Array<juce::String, juce::DummyCriticalSection>::Array (this=0xbffff2f4, other=@0xbffff2f4) at juce_Array.h:87
#4 0x001e1274 in juce::StringArray::StringArray (this=0xbffff2f4, other=@0xbffff2f4) at /Users/tom/Documents/development/juce/Builds/MacOSX/…/…/src/text/juce_StringArray.cpp:40
#5 0x0002daae in juce::AudioFormat::AudioFormat (this=0xbffff2ec, name=@0xbffff29c, extensions=@0xbffff2f4) at /Users/tom/Documents/development/juce/Builds/MacOSX/…/…/src/audio/audio_file_formats/juce_AudioFormat.cpp:517
#6 0x00027b0b in rec::audio::format::mpg123::Format::Format (this=0xbffff2ec) at Format.h:17
#7 0x000276b2 in rec::audio::format::mpg123::Format_IntFormat_Test::TestBody (this=0x14004d0) at /Users/tom/Documents/development/rec/unit-tests/…/src/rec/audio/format/mpg123/Format_test.cpp:17
#8 0x0054c568 in testing::Test::Run (this=0x14004d0) at gtest.cc:2095
#9 0x0054c653 in testing::internal::TestInfoImpl::Run (this=0x70d560) at gtest.cc:2314
#10 0x0054c796 in testing::TestCase::Run (this=0x70d620) at gtest.cc:2420
#11 0x0054caa6 in testing::internal::UnitTestImpl::RunAllTests (this=0x70b700) at gtest.cc:4024
#12 0x0054cc3c in testing::UnitTest::Run (this=0x578100) at gtest.cc:3687
#13 0x001eef2a in main (argc=1, argv=0xbffff4bc) at /Users/tom/Documents/development/gtest/xcode/…/src/gtest_main.cc:38


#2

Good puzzle! Took me about 10 minutes of confusion before I realised what’s actually going on here. It’s definitely not a bug in the library!

Do you want me to tell you what your mistake is, or would you rather have more time to figure it out…?


#3

My mistake? Hmm… sure!

If the container really has value copy semantics, it shouldn’t do that - passing the const & should be identical with returning the thing by value. It’s perfectly OK to return things by value that live on the stack, like for example std::vector, if their copy semantics are correct.

The whole point of having value copy semantics is that you can pass around and return these things “like they’re Plain Old Data” - there should be no way I should be able to manipulate it with returns so I end up with a pointer off into space.

I’m dying to see the answer though!


#4

It is your mistake, but it’s an easy one to make, and a very hard one to spot!

You need to think a bit more laterally… It has nothing at all to do with StringArray, Array, or any of the container classes…


#5

Well, hmm.

Sorry, I just don’t see it. Unless I’m missing something dramatic, this would work if I replaced String Array with int, std::string, or std::vectorstd::string. It should be fine that there are no static variables holding the initial characters, or that I’m creating and returning something immediately on the stack, if the container really has copy semantics.

If you’re thinking that passing the const pointer to the implied local variable is what’s incorrect, I assure you that it shouldn’t be - this idiom is very common in C++ and it works fine in STL and other containers. The local variable gets destroyed after the function call is over…

So, no, I’m stuck! What’s the answer?!

EDIT: oh, the embarrassment. I have a local function that’s the same name as a method. What fools, etc.


#6

On the other hand, I have now been promoted to Juce Obsessive, so perhaps it was worth it.


#7

Ah, I knew you’d get it in the end!

It’s a terrific bug though - reminds me of those adverts that Gimpel used to publish in Dr Dobbs every week, where you had to spot the obscure mistake.


#8

If you like this sort of thing, and you have any liking for Java at all, Bloch and Gafter’s Java Puzzlers is incredibly funny this way… I thought it was going to be dry but quite the reverse, it’s devilish - I saw Gafter give a talk on it and the audience booed and jeered at the results of some of his problems (all in good fun, of course).

The sobering part is that he claims that every bug there was abstracted from production code. Not all the problems are directly applicable to C++, of course… but yes, it includes a shadowing puzzler.


#9

You’re taking a reference on a non-const temporary object in the constructor (so the compiler doesn’t ensure object lifetime).

In general, it’s really not a good idea to return StringArray, as it’s a very weighty object.
For what you’re doing, you should create a static const object, and return a ref to this instead (like you did in your workaround).


#10

Maybe I’m misunderstanding you, but I don’t think there are any errors in there. Sure, it’s not super-efficient, but it should work just fine…


#11

I haven’t looked closely to the given example code, but usually this pattern is not recommanded:

class A
{
   A ([const] B & ref);
   void someMethod() = 0;
}

B getB();
A a (getB());
// Later
a.someMethod();

Since you don’t know if the object (B) will still exist when calling someMethod, the reference will/might point to garbish at that time (as in the given example).
Seems like AudioFormat is taking a ref on the string array, and might use it later on.


#12

Um… depends entirely on what class A actually does with its reference - if it keeps the reference and uses it later, then of course that’s a dangling pointer, but if it takes a copy of the object, like the AudioFormat class does, then it’s all fine. I certainly don’t think what you’ve written there is in any way a bad pattern, I must have written thousands of constructors that look like that!


#13

Sure. That was the point, if you don’t know what the class does, you shouldn’t pass a temporary to it since it can crash. (I’ve used references as they are a bit more vicious than pointers, the same example with pointers would have been obvious).
The usual “unofficial” convention is that a constructor taking “const A & a” will copy the object “a”, but the standard doesn’t ensure this at all. From the compiler point of view, it’s exactly like if it was written “A * a”.

Anyway, the issue from the backtrace looks like the StringArray used is being copied, and that the source object’s internal pointers is dangling. Somehow it looks like either the pointers points to (released) stack data, either the object was temporary and is now deleted. I don’t really know if Juce is still using COW semantics for its strings, and if it’s duplicating the text when using String (const tchar* const text) construction, but if it’s not the case, then the StringArray’s strings points to (released) stack data.


#14

We figured out the real problem quite a few posts back, you know…! The code is all 100% fine, but his choice of function name clashed with a base class method, and that was getting called instead, returning an array that hadn’t yet been constructed…