apt fails a test from the testsuite on ppc64el when built with gcc-5 and -O3

[Matthias Klose]

to reproduce:

 - wily chroot
 - PPA https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/landing-016/
   (this holds gcc-5 as the default, branch 20150711).
 - dpkg-buildpackage -B

note that explicitly using the gcc-5/g++-5 from wily doesn't show the test failure. the only difference in the configuration is that the one in wily is built using the old libstdc++ ABI and the one in the PPA with the dual ABI defaulting to new/c++11.

building with -O2 instead of -O3 works around the issue.

[ RUN ] CDROMTest.FindPackages
/scratch/doko/tmp/apt-1.0.9.10ubuntu1/test/libapt/cdromfindpackages_test.cc:95: Failure
Value of: InfoDir
  Actual: ""
Expected: path + "/.disk/"
Which is: "/tmp/apt-tests-findpackage.gpopMw/.disk/"
[ FAILED ] CDROMTest.FindPackages (4 ms)

[Matthias Klose (doko)]

the test fails too on amd64 with -O3

[Michael Vogt (mvo)]

I looked into this and the issue is the following:

There is a Cdrom wrapper:
"""
class Cdrom : public pkgCdrom {
   public:
      bool FindPackages(std::string const &CD,
     std::vector<std::string> &List,
     std::vector<std::string> &SList,
     std::vector<std::string> &SigList,
     std::vector<std::string> &TransList,
     std::string &InfoDir) {
  std::string const startdir = SafeGetCWD();
  EXPECT_FALSE(startdir.empty());
  bool const result = pkgCdrom::FindPackages(CD, List, SList, SigList, TransList, InfoDir, NULL, 0);
...
}
"""

and a unittest that calls it:
"""
TEST(CDROMTest,FindPackages)
{
   Cdrom cd;
  std::string InfoDir;
  EXPECT_TRUE(cd.FindPackages(path, Packages, Sources, Signatur, Translation, InfoDir));
...
  EXPECT_EQ(path + "/.disk/", InfoDir);
}
"""

The actual code for this is apt-pkg/cdrom.cc:
"""
bool pkgCdrom::FindPackages(string CD,
       vector<string> &List,
       vector<string> &SList,
       vector<string> &SigList,
       vector<string> &TransList,
       string &InfoDir, pkgCdromStatus *log,
       unsigned int Depth)
{
...
   if (DirectoryExists(".disk") == true)
   {
      if (InfoDir.empty() == true)
  InfoDir = CD + ".disk/";
   }
...
"""

So I suspect that the optimizer gets confused that InfoDir is a reference or it gets confused because InfoDir is not used in FindPackages anymore and it assumes its dead code.

I tried to create a simplified testcase but failed so far. Whats interessting is that if I add a std::cerr << "debug" line into cdrom.cc lines (or even a "CD = CD"):
"""
   if (DirectoryExists(".disk") == true)
   {
      if (InfoDir.empty() == true) {
std::cerr << "debug" << std::endl;
  InfoDir = CD + ".disk/";
}
   }
"""
it works (which indicates dead-code elimination to me).

[Matthias Klose (doko)]

just building apt-pkg/cdrom.cc with -O2 works

[William J. Schmidt (wschmidt-g)]

As a guess, is FindPackages being inlined? If so, and if the parameter passed in as &InfoDir isn't subsequently used, the compiler would be within its rights to remove the assignment to InfoDir. May want to check callers of FindPackages for this possibility.

[William J. Schmidt (wschmidt-g)]

Spent some time trying to reproduce this upstream. From the information given, I am unable to do so.

[Julian Andres Klode (juliank)]

DonKult added another workaround for this, which is now in 1.1.2:

http://anonscm.debian.org/cgit/apt/apt.git/commit/?id=0300f0077af832e87beb290f26b13404cab81fd3

[Matthias Klose (doko)]

seen again with https://launchpad.net/ubuntu/+source/apt/1.3~rc1ubuntu1
work around with https://launchpad.net/ubuntu/+source/apt/1.3~rc1ubuntu2

[Launchpad Janitor (janitor)]

This bug was fixed in the package apt - 1.3~rc1ubuntu2

---------------
apt (1.3~rc1ubuntu2) yakkety; urgency=medium

  * Don't build with -O3. See LP: #1473674.

 -- Matthias Klose <email address hidden> Fri, 12 Aug 2016 09:44:56 +0200

[Matthias Klose (doko)]

commit 9101c36c653cf90ccd133d6e85eca6b264de4b81
Author: David Kalnischkies <email address hidden>
Date: Fri Aug 12 19:11:01 2016 +0200

    drop "incorrect" const attribute from DirectoryExists

    Since its existence in 2010 DirectoryExists was always marked with this
    attribute, but for no real reason. Arguably a check for the existence of
    the file is not modifying global state, so theoretically this shouldn't
    be a problem. It is wrong from a logical point of view through as
    between two calls the directory could be created so the promise we made
    to the compiler that it could remove the second call would be wrong, so
    API wise it is wrong.

    The problem for the compiler is a lot more likely through in the
    underlying implementation of stat() and co, so for both reasons lets
    drop this attribute here and be (hopefully) done with it forever.

    It's a bit mysterious that this is only observeable on ppc64el and can be
    fixed by reordering code ever so slightly, but in the end its more our
    fault for adding this attribute than the compilers fault for doing
    something silly based on the attribute.

    LP: 1473674

diff --git a/apt-pkg/contrib/fileutl.h b/apt-pkg/contrib/fileutl.h
index 4a1676d..15665f8 100644
--- a/apt-pkg/contrib/fileutl.h
+++ b/apt-pkg/contrib/fileutl.h
@@ -164,7 +164,7 @@ bool RemoveFile(char const * const Function, std::string const &FileName);
 int GetLock(std::string File,bool Errors = true);
 bool FileExists(std::string File);
 bool RealFileExists(std::string File);
-bool DirectoryExists(std::string const &Path) APT_CONST;
+bool DirectoryExists(std::string const &Path);
 bool CreateDirectory(std::string const &Parent, std::string const &Path);
 time_t GetModificationTime(std::string const &Path);
 bool Rename(std::string From, std::string To);