[SERVER-12663] Storage for DocumentStorage::emptyDoc is not guaranteed to have suitable alignment Created: 09/Feb/14  Updated: 05/Feb/16  Resolved: 22/Dec/15

Status: Closed
Project: Core Server
Component/s: Internal Code
Affects Version/s: None
Fix Version/s: 3.3.0

Type: Bug Priority: Major - P3
Reporter: Andrew Morrow (Inactive) Assignee: Andrew Morrow (Inactive)
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Backwards Compatibility: Fully Compatible
Operating System: ALL
Sprint: Platforms E (01/08/16)
Participants:

 Description   

The following code assumes that the emptyBytes char array is suitably aligned to store an object of type DocumentStorage, but no alignment is enforced:

        static const DocumentStorage& emptyDoc() {
            static const char emptyBytes[sizeof(DocumentStorage)] = {0};
            return *reinterpret_cast<const DocumentStorage*>(emptyBytes);
        }

The code is also a strict aliasing violation.



 Comments   
Comment by Githook User [ 22/Dec/15 ]

Author:

{u'username': u'acmorrow', u'name': u'Andrew Morrow', u'email': u'acm@mongodb.com'}

Message: SERVER-12663 Make DocumentStorage::emptyDoc return a correctly aligned object
Branch: master
https://github.com/mongodb/mongo/commit/affd733b7b9ddad7f2501447569b687d3b81f9fa

Comment by Reno Reckling [ 11/Feb/14 ]

If performance is that important here, something like that would work:

class A {
    public:
    int x, y, z, b, c, d;
    A() : x(0) {};
    static const A& empty();
};
 
static const A emptyA;
 
const A& A::empty() {
    return emptyA;
};
 
int main() {
    const A a = A::empty();
    return 0;
}

Afaik global static class initialization is guaranteed to run before thread creation and even main().
That would make it thread safe.
Apart from the global variable ugliness of course.

Comment by Andrew Morrow (Inactive) [ 11/Feb/14 ]

Unfortunately, the code above is not thread-safe in C++03. While some compilers, like GCC, will automatically emit thread-safe code for function scoped statics unless asked not to, we cannot rely on that behavior since other compilers act differently. C++11 would require thread-safe initialization of the static here in all cases, however we would need to understand the cost of that thread-safety to make a decision, and we cannot yet require C++11.

Comment by Reno Reckling [ 11/Feb/14 ]

Maybe something like that:

static const DocumentStorage& emptyDoc() {
    static const DocumentStorage storage;
    return storage;
}

Would be simple enough?

The assembly of a similar case suggests that the object is just constructed once and reused on subsequent calls if compiled with -O2.

Comment by Reno Reckling [ 11/Feb/14 ]

I just realized that the old solution just hands out a static zeroed out field which should not get freed upon return...
So ignore what my previous remark.

Comment by Reno Reckling [ 11/Feb/14 ]

Doesn't that also return a reference to stack space?
The DocumentStorage zero-initialized all fields by default anyway, so why not just do something like:

static const DocumentStorage& emptyDoc() {
    DocumentStorage* storage = new DocumentStorage();
    return *storage;
}

Should be zeroed out via default constructor, properly aligned and not breaking strict aliasing. Or am i missing something?

Generated at Thu Feb 08 03:29:11 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.