Use templates for the base types to factorize code #186

robUx4 · 2023-12-26T15:08:19Z

A lot of the logic is the same between classes. We can factorize the code once we have factorized the storage (and initialization) types in a template.

The default value is moved to a EbmlCallbacksDefault class for the types that can have a default value. So the default value is not copied in each instance of every object.

~~This was done on top of #173 but maybe it works on its own.~~

ebml/EbmlElement.h

robUx4 · 2023-12-28T16:08:12Z

I need to update libmatroska to build with this branch

mbunkus

Doesn't merge with the current master at the moment. Can you please rebase? I'll review then.

robUx4 · 2023-12-29T06:42:10Z

Doesn't merge with the current master at the moment. Can you please rebase? I'll review then.

As soon as it works in libmatroska :)

mbunkus · 2023-12-29T12:09:53Z

ebml/EbmlElement.h

@@ -220,6 +277,7 @@ class EBML_DLL_API EbmlCallbacks {
        inline EbmlElement & NewElement() const { return Create(); }
        /// is infinite/unknown size allowed
        inline bool CanHaveInfiniteSize() const { return CanInfinite; }
+        virtual const bool HasDefault() const { return false; }


Compiler warning:

lib/libebml/ebml/EbmlElement.h:280:17: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers] virtual const bool HasDefault() const { return false; }

Just change it to virtual bool HasDefault() const both here & in EbmlCallbacksDefault.

Additional compiler warning:

lib/libebml/ebml/EbmlElement.h:262:20: warning: 'libebml::EbmlCallbacks' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor] class EBML_DLL_API EbmlCallbacks {

Adding a simple

virtual ~EbmlCallbacks() {}

seems to get rid of them, but at the cost of a compiler error in various later source files, e.g. EbmlDummy.cpp:

lib/libebml/src/EbmlDummy.cpp:13:1: fatal error: constexpr variable cannot have non-literal type 'const EbmlCallbacks' DEFINE_EBML_CLASS_ORPHAN(EbmlDummy, 0xFF, 1, "DummyElement") ^ lib/libebml/ebml/EbmlElement.h:150:60: note: expanded from macro 'DEFINE_EBML_CLASS_ORPHAN' #define DEFINE_EBML_CLASS_ORPHAN(x,id,idl,name) DEFINE_xxx_CLASS_ORPHAN(x,id,idl,name,GetEbmlGlobal_Context) ^ lib/libebml/ebml/EbmlElement.h:145:38: note: expanded from macro 'DEFINE_xxx_CLASS_ORPHAN' constexpr const EbmlCallbacks x::ClassInfos(x::Create, Id_##x, false, name, Context_##x); \ ^ lib/libebml/ebml/EbmlElement.h:274:13: note: 'EbmlCallbacks' is not literal because it has a user-provided destructor virtual ~EbmlCallbacks() {} ^

I fixed both. It's odd I don't get these warnings in clang.

Hmmm the warning: 'libebml::EbmlCallbacks' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor] warning is still there in revision 5b434b7.

BTW, I also compile with clang++, using the following warning/error options:

-Wall -Wno-comment -Wfatal-errors -fstack-protector-strong -O3 -g3 -std=c++17 -Wnon-virtual-dtor -Wextra -Wno-missing-field-initializers -Wunused -Wpedantic -Woverloaded-virtual -Wshadow -Qunused-arguments -Wno-self-assign -Wno-mismatched-tags -Wno-inconsistent-missing-override -Wno-potentially-evaluated-expression -Wno-extra-semi

Pretty sure that -Wnon-virtual-dtor turns on this particular warning.

Fixed. I also added #195 to make these issues more visible in the future.

mbunkus · 2023-12-30T14:26:53Z

I still get an error:

lib/libmatroska/src/KaxSemantic.cpp:113:1: fatal error: constexpr variable cannot have non-literal type 'const EbmlCallbacks'
DEFINE_MKX_BINARY_CONS(KaxSimpleBlock, 0xA3, 1, KaxCluster, "SimpleBlock")
^
lib/libmatroska/matroska/KaxDefines.h:30:32: note: expanded from macro 'DEFINE_MKX_BINARY_CONS'
    constexpr EbmlCallbacks a::ClassInfos(a::Create, Id_##a, false, e, Context_##a);
                               ^
lib/libebml/ebml/EbmlElement.h:274:13: note: 'EbmlCallbacks' is not literal because it has a non-trivial destructor
    virtual ~EbmlCallbacks() = default;
            ^
lib/libebml/ebml/EbmlElement.h:274:13: note: destructor for 'EbmlCallbacks' is not trivial because it is virtual
1 error generated.

robUx4 · 2023-12-30T14:33:18Z

I forgot to push the lastest version of the libmatroska counterpart: Matroska-Org/libmatroska#134
It should work now.

mbunkus · 2023-12-30T16:08:43Z

Thanks. Compiles now. But there are a ton of issues from crashing due to null pointer access to use of uninitialized values. I've fixed some of them locally, will try to look into the others over the next couple of days. Definitely not ready for merging yet.

mbunkus

There are at least three issues that I found. I couldn't really fix them as I didn't understand your new structure enough. Please look into the following:

nullptr arguments not checked

Operators & functions taking pointers are dereferencing the pointers, leading to invalid memory access or exceptions being thrown when nullptr is given as an argument. This immediately happens in operator == in both EbmlString & EbmlUnicodeString. I don't have the stack trace at hand, but both functions were called with a nullptr arg, causing std::string/std::wstring to throw an std::logic_error.

Not sure what the right way to handle nullptr is. Two possibilities:

Always return false, as in "must not be used that way, cannot be equal"
Check if stored Value is empty & return true if so, as in "no string and empty string are the same"

Another function that comes to mind here is EbmlElementDefaultSameStorage::IsSmallerThan.

reads from uninitialized values

Using valgrind I can see tons of errors of the form of making decisions based on uninitialized Value member variables, mostly in EbmlUInteger. This is unsurprising as the EbmlElementDefaultSameStorage and EbmlElementDefaultStorage don't initialize their Value member variable, neither statically in the class definition, nor in the constructor. The constructor in the original EbmlUInteger class from before the changes initialized their value member to the class's default value according to the specs, or to 0 (or the variable's equivalent) if there was no default.

This definitely affects EbmlUInteger, EbmlSInteger, EbmlFloat, EbmlDate as they have trivial member variables, which are only initialized if the code requests it.

It likely does not affect EbmlString, EbmlUnicodeString, EbmlBinary, EbmlCrc32 as those use non-trivial types which do get initialized via their default constructor.

Please take a look at those issues.

robUx4 · 2023-12-31T07:22:01Z

I simplified the PR to not include the code factorization to make it easier to spot the regressions. The libmatroska counterpart is still needed with this PR.

robUx4 · 2023-12-31T07:47:24Z

I added some tests for strings with and without a default value and did hit some null pointer dereferencing. It was using the default value when there isn't one. It may also be the valgrind issues you saw. The default value should never be used if the default class says it has no default value.

So I think this branch should be safe now.

mbunkus · 2023-12-31T19:38:25Z

I cannot test the branch in its current form. I need to be able to query an existing element if it has a default value. In 1.x I can do that via elt.DefaultISset(), which has been removed, which is fine. In earlier iterations of this PR I could go via the EbmlCallbacks with elt.GetClassInfo().HasDefault(), which was fine, too.

However, this current iteration doesn't have a way to retrieve the EbmlCallbacks anymore.

robUx4 · 2024-01-01T08:01:18Z

Rebased on top of #204. I added the DefaultIsSet() call to the macro tests in #204 and turned it into the new call to use in this PR: GetElementSpec().HasDefault().

This PR consists of 2 PRs on top of #204.

robUx4 · 2024-01-01T10:57:34Z

I reworked the code further. I am now testing default values on an EbmlElement in the tests (ie without knowing anything about the underlying class).

There is no longer a type that holds the default value if there is one. There's one EbmlCallbacksWithDefault template that holds the acutal default value. Types without a default value use EbmlCallbacksDefault.

mbunkus · 2024-01-01T12:24:59Z

Thanks. With elt.ElementSpec().HasDefault() I can now compile this branch (together with the corresponding one for libmatroska). Unfortunately most of my test cases still fail due to different checksums (at least there are no more crashes as far as I can see; so the nullptr comparison seems to have been solved). For example, here's how the EBML Head looks like with a file produced by a mkvmerge compiled with v1.x:

+ EBML head
|+ EBML version: 1
|+ EBML read version: 1
|+ Maximum EBML ID length: 4
|+ Maximum EBML size length: 8
|+ Document type: matroska
|+ Document type version: 4
|+ Document type read version: 1

Here's what the same code compiled with branch default_init/11 looks like:

+ EBML head
|+ EBML version: 0
|+ EBML read version: 0
|+ Maximum EBML ID length: 0
|+ Maximum EBML size length: 0
|+ Document type: matroska
|+ Document type version: 4
|+ Document type read version: 1

Another interesting (but wrong) effect is that the cues suddenly contain "cue codec state" elements, even if the file doesn't contain any codec state elements! They're just… there suddenly. Again I'm comparing files created with the same mkvmerge code, just compiled with different libebml/libmatroska releases.

It seems that the initial values are still not initialized. Running the mkvmerge from default_init/11 with valgrind, I get lots of warnings such as this:

==986973== Conditional jump or move depends on uninitialised value(s)
==986973==    at 0x7290EC: libebml::EbmlUInteger::UpdateSize(std::function<bool (libebml::EbmlElement const&)>, bool) (lib/libebml/src/EbmlUInteger.cpp:59)
==986973==    by 0x725D44: libebml::EbmlMaster::UpdateSize(std::function<bool (libebml::EbmlElement const&)>, bool) (lib/libebml/src/EbmlMaster.cpp:112)
==986973==    by 0x7235FD: libebml::EbmlElement::Render(libebml::IOCallback&, std::function<bool (libebml::EbmlElement const&)>, bool, bool) (lib/libebml/src/EbmlElement.cpp:486)
==986973==    by 0x3FE733: render_ebml_head (src/merge/output_control.cpp:492)
==986973==    by 0x3FE733: render_headers (???:581)
==986973==    by 0x3FE733: create_next_output_file() (???:1425)
==986973==    by 0x39DDDD: main (src/merge/mkvmerge.cpp:3206)

I definitely do not have those warnings when running valgrind on the "good" mkvmerge compiled against v1.x.

These types of decisions based on uninitialized values could easily explain the behavior mentioned above with the "cue codec state" elements suddenly appearing.

robUx4 · 2024-01-01T12:41:07Z

Thanks for all the tests. If the header is broken it's definitely something that can be tested in libebml. I'll give this a try.

Can you check that #204 works for you ? It will help reduce the scope of this PR.

mbunkus · 2024-01-01T12:59:01Z

Done! #204 is fine & can be merged.

It's mandatory since 8494914 and always stored in the EbmlElement.

So the default value is hardcoded in the class semantic info and not set in the constructor. A new EbmlElement subclass is created for EbmlElement types that may have a default value. An IsSameValue() method is added to compare an element to a value. This is used by the EbmlElementDefault class to tell if the element value is the same as the default when there is one.

robUx4 · 2024-01-01T14:02:06Z

I found the issue. The old constructor with a default value would initialize the element to its default value. IMO the element should not be initialized until it's manually done. If not it has its default value to use or should not be used at all.

For I went with the forced initialization. Only for elements with a default value, the other ones should not be used without setting a value (or their size is 0 = default value for the type).

Now it passes the test from #206.

mbunkus

Thanks! Now it passes all of my tests as well. Nice work!

Can be merged (along with the corresponding branch for libmatroska).

robUx4 · 2024-01-01T15:25:14Z

Awesome. Thanks for all the testing.

robUx4 commented Dec 27, 2023

View reviewed changes

ebml/EbmlElement.h Outdated Show resolved Hide resolved

robUx4 force-pushed the default_init/11 branch 2 times, most recently from fa30c2f to 4f3de95 Compare December 28, 2023 15:23

robUx4 changed the title ~~[RFC] Use templates for the base types to factorize code~~ Use templates for the base types to factorize code Dec 28, 2023

robUx4 requested a review from mbunkus December 28, 2023 15:30

mbunkus requested changes Dec 28, 2023

View reviewed changes

robUx4 force-pushed the default_init/11 branch from 4f3de95 to 074b33d Compare December 29, 2023 10:05

robUx4 mentioned this pull request Dec 29, 2023

Use templates for the base types to factorize code Matroska-Org/libmatroska#134

Merged

robUx4 force-pushed the default_init/11 branch 2 times, most recently from 742314a to d0af2a8 Compare December 29, 2023 10:51

mbunkus requested changes Dec 29, 2023

View reviewed changes

robUx4 force-pushed the default_init/11 branch 2 times, most recently from 3e87c7c to 5b434b7 Compare December 29, 2023 14:13

robUx4 mentioned this pull request Dec 29, 2023

Make the Value private #194

Merged

robUx4 force-pushed the default_init/11 branch from 5b434b7 to d2a17a3 Compare December 30, 2023 09:40

mbunkus requested changes Dec 30, 2023

View reviewed changes

robUx4 mentioned this pull request Dec 31, 2023

Use templates to factorize generic code #200

Merged

robUx4 force-pushed the default_init/11 branch from d2a17a3 to da4da9b Compare December 31, 2023 07:20

robUx4 force-pushed the default_init/11 branch from da4da9b to f797165 Compare December 31, 2023 07:45

robUx4 force-pushed the default_init/11 branch 2 times, most recently from 280eddf to 19ad33a Compare December 31, 2023 11:18

robUx4 mentioned this pull request Jan 1, 2024

Misc. cleanups #204

Merged

robUx4 force-pushed the default_init/11 branch from 19ad33a to 836d4ae Compare January 1, 2024 07:59

robUx4 force-pushed the default_init/11 branch 2 times, most recently from 0eac624 to 288a6b6 Compare January 1, 2024 10:57

robUx4 mentioned this pull request Jan 1, 2024

add a test to write/read an EBML header #206

Merged

robUx4 added 2 commits January 1, 2024 14:58

get access to the EbmlCallbacks of all EbmlElement

81eb9e1

It's mandatory since 8494914 and always stored in the EbmlElement.

robUx4 force-pushed the default_init/11 branch from 288a6b6 to d4f9bc8 Compare January 1, 2024 13:59

mbunkus approved these changes Jan 1, 2024

View reviewed changes

robUx4 merged commit 51c2767 into Matroska-Org:master Jan 1, 2024

robUx4 deleted the default_init/11 branch January 1, 2024 15:25

robUx4 mentioned this pull request Feb 26, 2024

Make the default value part of the semantic #154

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use templates for the base types to factorize code #186

Use templates for the base types to factorize code #186

robUx4 commented Dec 26, 2023 •

edited

Loading

robUx4 commented Dec 28, 2023

mbunkus left a comment

robUx4 commented Dec 29, 2023

mbunkus Dec 29, 2023

mbunkus Dec 29, 2023

robUx4 Dec 29, 2023

mbunkus Dec 29, 2023

mbunkus Dec 29, 2023

robUx4 Dec 30, 2023

mbunkus commented Dec 30, 2023

robUx4 commented Dec 30, 2023

mbunkus commented Dec 30, 2023

mbunkus left a comment

robUx4 commented Dec 31, 2023

robUx4 commented Dec 31, 2023

mbunkus commented Dec 31, 2023

robUx4 commented Jan 1, 2024 •

edited

Loading

robUx4 commented Jan 1, 2024

mbunkus commented Jan 1, 2024

robUx4 commented Jan 1, 2024

mbunkus commented Jan 1, 2024

robUx4 commented Jan 1, 2024

mbunkus left a comment

robUx4 commented Jan 1, 2024

Use templates for the base types to factorize code #186

Use templates for the base types to factorize code #186

Conversation

robUx4 commented Dec 26, 2023 • edited Loading

robUx4 commented Dec 28, 2023

mbunkus left a comment

Choose a reason for hiding this comment

robUx4 commented Dec 29, 2023

mbunkus Dec 29, 2023

Choose a reason for hiding this comment

mbunkus Dec 29, 2023

Choose a reason for hiding this comment

robUx4 Dec 29, 2023

Choose a reason for hiding this comment

mbunkus Dec 29, 2023

Choose a reason for hiding this comment

mbunkus Dec 29, 2023

Choose a reason for hiding this comment

robUx4 Dec 30, 2023

Choose a reason for hiding this comment

mbunkus commented Dec 30, 2023

robUx4 commented Dec 30, 2023

mbunkus commented Dec 30, 2023

mbunkus left a comment

Choose a reason for hiding this comment

nullptr arguments not checked

reads from uninitialized values

robUx4 commented Dec 31, 2023

robUx4 commented Dec 31, 2023

mbunkus commented Dec 31, 2023

robUx4 commented Jan 1, 2024 • edited Loading

robUx4 commented Jan 1, 2024

mbunkus commented Jan 1, 2024

robUx4 commented Jan 1, 2024

mbunkus commented Jan 1, 2024

robUx4 commented Jan 1, 2024

mbunkus left a comment

Choose a reason for hiding this comment

robUx4 commented Jan 1, 2024

robUx4 commented Dec 26, 2023 •

edited

Loading

robUx4 commented Jan 1, 2024 •

edited

Loading