DynamoRIO / dynamorio

Dynamic Instrumentation Tool Platform
Other
2.61k stars 554 forks source link

gcc_struct attribute directive ignored warning in AArch64 builds #6604

Closed AssadHashmi closed 7 months ago

AssadHashmi commented 7 months ago

The AArch64 build throws up 267 warnings in the elfutils submodule to do with lack of GCC compiler support for __attribute__ ((packed, gcc_struct)), e.g.

In file included from /work/sources/dynamorio/third_party/elfutils/libdwelf/libdwelfP.h:33,
                 from /work/sources/dynamorio/third_party/elfutils/libdwelf/dwelf_dwarf_gnu_debugaltlink.c:33:
/work/sources/dynamorio/third_party/elfutils/libdw/libdwP.h:279:1: warning: ‘gcc_struct’ attribute directive ignored [-Wattributes]
  279 | } attribute_packed;
      | ^

The gcc_struct attribute modifier in ext/drsyms/elfutils/config.h appears to be selectable using HAVE_GCC_STRUCT:

#ifdef HAVE_GCC_STRUCT
#define attribute_packed \
  __attribute__ ((packed, gcc_struct))
#else
#define attribute_packed \
  __attribute__ ((packed))
#endif

Is the warning telling us something we need to action or a cautionary message we can ignore? Removing it on AArch64 makes no difference to test results.

derekbruening commented 7 months ago
$ git grep HAVE_GCC_STRUCT
ext/drsyms/elfutils/config.h:#define HAVE_GCC_STRUCT 1

To convert the autoconf to cmake I took the config.h from my machine, which looks like it does not apply everywhere. In this case I would say let's just set that to 0 in the checked-in config.h.

AssadHashmi commented 7 months ago

I've done this locally so it only applies to AArch64:

index 8b9627911..15090b59c 100644
--- a/ext/drsyms/CMakeLists.txt
+++ b/ext/drsyms/CMakeLists.txt
@@ -210,6 +210,9 @@ elseif (UNIX)
         COMPILE_DEFINITIONS
         "_GNU_SOURCE;HAVE_CONFIG_H;_FORTIFY_SOURCE=3;PIC;SHARED;SYMBOL_VERSIONING;malloc=__wrap_malloc;calloc=__wrap_calloc;realloc=__wrap_realloc;free=__wrap_free;strdup=__wrap_strdup"
         COMPILE_FLAGS "-std=gnu99 -Wall -g -O2 -fPIC")
+      if (NOT DR_HOST_AARCH64)
+        set_property(TARGET ${lib}_pic APPEND_STRING PROPERTY COMPILE_DEFINITIONS "ENABLE_GCC_STRUCT")
+      endif ()
       DR_export_target(${lib}_pic)
       install_exported_target(${lib}_pic ${INSTALL_EXT_LIB})
       copy_target_to_device(${lib}_pic "${location_suffix}")
diff --git a/ext/drsyms/elfutils/config.h b/ext/drsyms/elfutils/config.h
index 9bc0f412c..9c0bb63ec 100644
--- a/ext/drsyms/elfutils/config.h
+++ b/ext/drsyms/elfutils/config.h
@@ -72,7 +72,9 @@
 #define HAVE_FALLTHROUGH 1

 /* Defined if __attribute__((gcc_struct)) is supported */
-#define HAVE_GCC_STRUCT 1
+#ifdef ENABLE_GCC_STRUCT
+#    define HAVE_GCC_STRUCT 1
+#endif

 /* Define to 1 if you have the `getrlimit' function. */
 #define HAVE_GETRLIMIT 1
AssadHashmi commented 7 months ago

https://github.com/DynamoRIO/dynamorio/pull/6605 approved.