[Scummvm-git-logs] scummvm master -> e6195c59d213ed3904be001b01fd8ddec3689b4c

[lephilousophe]

This automated email contains information about 2 new commits which have been
pushed to the 'scummvm' repo located at https://api.github.com/repos/scummvm/scummvm .

Summary:
b04cfecccd GRIM: Use concrete types to avoid GCC warning
e6195c59d2 GRIM: Move LangFilter destructor


Commit: b04cfecccd49f0a9bda95d5dba6f8b0c0aacd7a3
    https://github.com/scummvm/scummvm/commit/b04cfecccd49f0a9bda95d5dba6f8b0c0aacd7a3
Author: Orgad Shaneh (orgads at gmail.com)
Date: 2025-05-29T09:24:52+02:00

Commit Message:
GRIM: Use concrete types to avoid GCC warning

Fixes the following GCC warning on release build:

common/archive.h:353: warning: array subscript 'Common::SearchSet::__as_base [0]' is partly outside array bounds of 'unsigned char [13104]' [-Warray-bounds=]
In file included from engines/grim/update/update.cpp:23:
In destructor 'virtual Common::SearchSet::~SearchSet()',
    inlined from 'virtual Common::SearchSet::~SearchSet()' at common/archive.h:353:34,
    inlined from 'Common::Archive* Grim::loadUpdateArchive(Common::SeekableReadStream*)' at engines/grim/update/update.cpp:40:10:
common/archive.h:353:41: warning: array subscript 'Common::SearchSet::__as_base [0]' is partly outside array bounds of 'unsigned char [13104]' [-Warray-bounds=]
  353 |         virtual ~SearchSet() { clear(); }
      |                                         ^

Explanation by @lephilosophe:
First, in the source code, we see that the warning matches a call to
LangFilter constructor and its (virtual) destruction.

LangFilter only inherits on Archive and nothing else.

Its destructor is located in a separate translation unit and cannot be
inlined. No SearchSet object is at stake here...

When you look at the produced code, you see that SearchSet vtable and
SearchSet destructor are referenced and it's where the madness begins.

The (generated) code starts by checking if the destructor of our
LangFilter object is the SearchSet destructor (by matching the
address in the vtable with the address of the SearchSet::~SearchSet),
if it's the case, the inline SearchSet destructor is executed inlined.

And that's where the array bound warning triggers: because the inlined
code tries to access to memory which has never been allocated.

But... this code will NEVER EVER been executed because the object is not
a SearchSet object.

Now, from where comes this GCC hallucination? From the -fdevirtualize
optimization enabled by -O2.

If you build after configuring using the following command-line:
CXXFLAGS="-fno-devirtualize" ./configure --enable-release,
you don't get the cryptic warning anymore, because the offending
code is not generated anymore.

Changed paths:
    engines/grim/update/update.cpp


diff --git a/engines/grim/update/update.cpp b/engines/grim/update/update.cpp
index c21bd5be8f7..1d03e3eb956 100644
--- a/engines/grim/update/update.cpp
+++ b/engines/grim/update/update.cpp
@@ -20,7 +20,6 @@
  */
 
 #include "common/stream.h"
-#include "common/archive.h"
 
 #include "engines/grim/update/packfile.h"
 #include "engines/grim/update/mscab.h"
@@ -32,8 +31,8 @@ namespace Grim {
 
 Common::Archive *loadUpdateArchive(Common::SeekableReadStream *data) {
 	Common::SeekableReadStream *updStream = new PackFile(data);
-	Common::Archive *cab = new MsCabinet(updStream);
-	Common::Archive *update = new LangFilter(cab, g_grim->getGameLanguage());
+	MsCabinet *cab = new MsCabinet(updStream);
+	LangFilter *update = new LangFilter(cab, g_grim->getGameLanguage());
 
 	Common::ArchiveMemberList list;
 	if (update->listMembers(list) == 0) {


Commit: e6195c59d213ed3904be001b01fd8ddec3689b4c
    https://github.com/scummvm/scummvm/commit/e6195c59d213ed3904be001b01fd8ddec3689b4c
Author: Le Philousophe (lephilousophe at users.noreply.github.com)
Date: 2025-05-29T09:24:52+02:00

Commit Message:
GRIM: Move LangFilter destructor

This may allow inlining when the is deleted.

Changed paths:
    engines/grim/update/lang_filter.cpp
    engines/grim/update/lang_filter.h


diff --git a/engines/grim/update/lang_filter.cpp b/engines/grim/update/lang_filter.cpp
index a386178171c..186ac9fffc7 100644
--- a/engines/grim/update/lang_filter.cpp
+++ b/engines/grim/update/lang_filter.cpp
@@ -61,10 +61,6 @@ LangFilter::LangFilter(Common::Archive *arc, Common::Language lang) : _arc(arc)
 	}
 }
 
-LangFilter::~LangFilter() {
-	delete _arc;
-}
-
 bool LangFilter::hasFile(const Common::Path &path) const {
 	if (!_arc)
 		return false;
diff --git a/engines/grim/update/lang_filter.h b/engines/grim/update/lang_filter.h
index 3eaf9743307..e0a1560272f 100644
--- a/engines/grim/update/lang_filter.h
+++ b/engines/grim/update/lang_filter.h
@@ -30,7 +30,10 @@ namespace Grim {
 class LangFilter : public Common::Archive {
 public:
 	LangFilter(Common::Archive *arc, Common::Language lang);
-	~LangFilter();
+	~LangFilter() {
+		delete _arc;
+	}
+
 
 	// Common::Archive API implementation
 	bool hasFile(const Common::Path &path) const override;