[Libreoffice-commits] core.git: 2 commits - avmedia/source external/firebird

Stephan Bergmann sbergman at redhat.com
Tue Apr 17 05:48:49 UTC 2018 

 avmedia/source/viewer/mediawindow.cxx         |    8 ++++----
 external/firebird/ExternalProject_firebird.mk |    2 --
 external/firebird/asan.patch                  |   13 ++++++-------
 3 files changed, 10 insertions(+), 13 deletions(-)

New commits:
commit 0a910746b19f10f184f6ff8f41c868994a472ca9
Author: Stephan Bergmann <sbergman at redhat.com>
Date:   Mon Apr 16 16:48:41 2018 +0200

    Clarify checking o_pbLink directly, versus missing dereferencing
    
    But given 18ab7abaa9426cd27092125637fdf5fb849b4a04
    "MediaWindow::executeMediaURLDialog: add link button" documents that parameter
    as "if not 0, this is an 'insert' dialog", it looks plausible that all these
    places indeed want to check the pointer-to-bool for non-nullness, rather than
    checking the dereferenced bool.
    
    Change-Id: Ifbd5e4b711bee97f3ba0b6aab556f533574d21c6
    Reviewed-on: https://gerrit.libreoffice.org/52992
    Reviewed-by: Michael Stahl <Michael.Stahl at cib.de>
    Tested-by: Jenkins <ci at libreoffice.org>

diff --git a/avmedia/source/viewer/mediawindow.cxx b/avmedia/source/viewer/mediawindow.cxx
index 2b2f0937f562..0fd10f508bca 100644
--- a/avmedia/source/viewer/mediawindow.cxx
+++ b/avmedia/source/viewer/mediawindow.cxx
@@ -211,7 +211,7 @@ void MediaWindow::getMediaFilters( FilterNameVector& rFilterNameVector )
 
 bool MediaWindow::executeMediaURLDialog(weld::Window* pParent, OUString& rURL, bool *const o_pbLink)
 {
-    ::sfx2::FileDialogHelper        aDlg(o_pbLink
+    ::sfx2::FileDialogHelper        aDlg(o_pbLink != nullptr
             ? ui::dialogs::TemplateDescription::FILEOPEN_LINK_PREVIEW
             : ui::dialogs::TemplateDescription::FILEOPEN_SIMPLE,
             FileDialogFlags::NONE, pParent);
@@ -220,7 +220,7 @@ bool MediaWindow::executeMediaURLDialog(weld::Window* pParent, OUString& rURL, b
     static const char               aSeparator[] = ";";
     OUString                        aAllTypes;
 
-    aDlg.SetTitle( AvmResId( o_pbLink
+    aDlg.SetTitle( AvmResId( o_pbLink != nullptr
                 ? AVMEDIA_STR_INSERTMEDIA_DLG : AVMEDIA_STR_OPENMEDIA_DLG ) );
 
     getMediaFilters( aFilters );
@@ -261,7 +261,7 @@ bool MediaWindow::executeMediaURLDialog(weld::Window* pParent, OUString& rURL, b
     uno::Reference<ui::dialogs::XFilePicker3> const xFP(aDlg.GetFilePicker());
     uno::Reference<ui::dialogs::XFilePickerControlAccess> const xCtrlAcc(xFP,
             uno::UNO_QUERY_THROW);
-    if (o_pbLink)
+    if (o_pbLink != nullptr)
     {
         // for video link should be the default
         xCtrlAcc->setValue(
@@ -278,7 +278,7 @@ bool MediaWindow::executeMediaURLDialog(weld::Window* pParent, OUString& rURL, b
         const INetURLObject aURL( aDlg.GetPath() );
         rURL = aURL.GetMainURL( INetURLObject::DecodeMechanism::Unambiguous );
 
-        if (o_pbLink)
+        if (o_pbLink != nullptr)
         {
             uno::Any const any = xCtrlAcc->getValue(
                 ui::dialogs::ExtendedFilePickerElementIds::CHECKBOX_LINK, 0);
commit 25764ffd4db0e5db6f9cc9f3da8691e607f48b83
Author: Stephan Bergmann <sbergman at redhat.com>
Date:   Mon Apr 16 11:09:22 2018 +0200

    external/firebird: Better workaround for Clang alignment expectations
    
    8ea07101c1613d213fd7cea17f094a947b14cd00 "external/firebird: Work around
    operator new alignment violations" had misused DEBUG_GDS_ALLOC to work around
    the problem that recent Clang on Linux x86-64 assumes some storage allocated via
    Firebird's global operator new replacement to be 16-byte aligned, while Firebird
    only provides 8-byte alignment.
    
    This problem now also appears on macOS x86-64, at least with Apple's Clang
    version "Apple LLVM version 9.1.0 (clang-902.0.39.1)" from Xcode 9.3 (as well as
    with recent plain Clang trunk, towards Clang 7), when building --enable-
    optimized.  However, while the DEBUG_GDS_ALLOC hack would still cause
    ICU_convert_init (workdir/UnpackedTarball/firebird/temp/Release/intl/cv_icu.o)
    to not use movaps on erroneously assumed to be 16-byte aligned memory, it would
    cause strange failures on macOS (pos being out of bounds in traRpbList::PopRpb,
    workdir/UnpackedTarball/firebird/src/jrd/rpb_chain.cpp, in the invocation of
    isql during the build of external/firebird) that I haven't tracked down further.
    
    As it happens, the recently added 14955ed91b282ccbb395cb47c6d76e3b42b34748
    "external/firebird: Support Clang ASan" provides a different workaround for the
    underlying problem that appears to work well on both Linux and macOS x86-64,
    reusing USE_ASAN also in these cases to shut down most of Firebird's own memory
    management.
    
    I assume that affected Clang are plain Clang >= 4 (as I'd mentioned in my
    <https://sourceforge.net/p/firebird/mailman/message/35671804/> "Re: [Firebird-
    devel] alloc.h global operator new replacement violating alignment
    requirements") and Apple Clang >= 9 (for which __apple_build_version__ is
    defined).
    
    Because DEBUG_GDS_ALLOC is no longer passed in from the outside, its setting in
    external/firebird/asan.patch can be simplified (cf. commit message of
    14955ed91b282ccbb395cb47c6d76e3b42b34748).
    
    (The given scenario in ICU_convert_init involves an allocation of 24 bytes,
    where Clang may or may not be allowed to assume 16-byte alignment, see
    <http://lists.llvm.org/pipermail/cfe-dev/2018-April/057669.html> "Re: [cfe-dev]
    operator new alignment assumptions".  However, as reported at
    <https://sourceforge.net/p/firebird/mailman/message/35671750/> "Re: [Firebird-
    devel] alloc.h global operator new replacement violating alignment
    requirements", Firebird only supports 8-byte alignment, which would definitely
    be wrong in a similar scenario where the requested allocation was a multiple of
    16 bytes.)
    
    Change-Id: I48884f9d008eaeaea369850e24f05b8806f9b676
    Reviewed-on: https://gerrit.libreoffice.org/52956
    Tested-by: Jenkins <ci at libreoffice.org>
    Reviewed-by: Stephan Bergmann <sbergman at redhat.com>

diff --git a/external/firebird/ExternalProject_firebird.mk b/external/firebird/ExternalProject_firebird.mk
index 5ca8c0b01418..d370df0d83ae 100644
--- a/external/firebird/ExternalProject_firebird.mk
+++ b/external/firebird/ExternalProject_firebird.mk
@@ -58,8 +58,6 @@ $(call gb_ExternalProject_get_state_target,firebird,build):
 			" \
 		&& export CXXFLAGS=" \
 			$(if $(filter MSC,$(COM)),$(if $(MSVC_USE_DEBUG_RUNTIME),-DMSVC_USE_DEBUG_RUNTIME)) \
-			$(if $(filter LINUX/X86_64/TRUE,$(OS)/$(CPUNAME)/$(COM_IS_CLANG)), \
-				-DDEBUG_GDS_ALLOC) \
 			$(if $(HAVE_GCC_FNO_SIZED_DEALLOCATION),-fno-sized-deallocation -fno-delete-null-pointer-checks) \
 			$(if $(SYSTEM_BOOST),$(BOOST_CPPFLAGS), \
 				$(BOOST_CPPFLAGS) \
diff --git a/external/firebird/asan.patch b/external/firebird/asan.patch
index 4c40f52bfc87..b6ecec8f4caf 100644
--- a/external/firebird/asan.patch
+++ b/external/firebird/asan.patch
@@ -204,7 +204,7 @@
  #ifdef DEBUG_GDS_ALLOC
 --- src/include/firebird.h
 +++ src/include/firebird.h
-@@ -38,10 +38,19 @@
+@@ -38,8 +38,17 @@
  #include "gen/autoconfig.h"
  #endif
  
@@ -213,17 +213,16 @@
 +#define USE_ASAN
 +#endif
 +#endif
++#if defined __clang__ && (defined __apple_build_version__ ? __clang_major__ >= 9 : __clang_major__ >= 4)
++#define USE_ASAN
++#endif
 +
  // Using our debugging code is pointless when we may use Valgrind features
- #if defined(DEV_BUILD) && !defined(USE_VALGRIND)
+-#if defined(DEV_BUILD) && !defined(USE_VALGRIND)
++#if defined(DEV_BUILD) && !(defined(USE_VALGRIND) || defined(USE_ASAN))
  #define DEBUG_GDS_ALLOC
  #endif
-+#if defined USE_ASAN
-+#undef DEBUG_GDS_ALLOC
-+#endif
  
- #if defined(WIN_NT)
- #define FB_DLL_EXPORT __declspec(dllexport)
 --- src/jrd/SimilarToMatcher.h
 +++ src/jrd/SimilarToMatcher.h
 @@ -338,7 +338,7 @@

More information about the Libreoffice-commits mailing list