Re: [DynInst_API:] the PRIx64 commit breaks the build with some <inttypes.h>

[Tim Haines <thaines.astro@xxxxxxxxx>]

There is one use in instructionAPI/src/ArchSpecificFormatters.C that's been there since 2017 and Compiler Explorer says it works, so we should be good on 4.8.4 compatibility. It's also building on targeted platforms successfully. As long as it works on your end, we can merge it.

Thanks.

- Tim

On Wed, Aug 7, 2019 at 4:28 PM Mark W. Krentel <krentel@xxxxxxxx> wrote:

Ok, I'll test it. One quick thing you could do now is scan the code
and see if std::to_string() is something you already use or not.

If you're already using it in several places, then probably it won't
be a problem.

--Mark

On 08/07/19 14:51, Tim Haines wrote:

Hi, Mark.

I created a PR to fix this, could you test on your machines?

>Before you commit anything, test it with gcc 4.8.x. I'm not sure if the 4.8 compiler implements 100% of C++11.

I don't have any systems with a gcc that old. If you do, could you test there?

Thanks.

Â- Tim

On Wed, Aug 7, 2019 at 2:03 PM Mark W. Krentel <krentel@xxxxxxxx> wrote:

Tim,

Based on the contents of the <inttypes.h> file, I don't see where this
would be limited to 32-bit.

I have seen this on more than just power8. With RH 6.x, it's "broken"
on both x86_64 and power7, both with glibc-headers-2.12. With RH 7.x,
it's broken on power8 but not x86_64, both with glibc-headers-2.17.

Where "broken" means that <inttypes.h> won't define PRIx64 without
__STDC_FORMAT_MACROS.

I guess this is old code and the #if was removed at some point, but
removed on x86 before ppc. But that may be moot now.

One thing about std::to_string() and C++11. Before you commit
anything, test it with gcc 4.8.x. I'm not sure if the 4.8 compiler
implements 100% of C++11.

--Mark

On 08/07/19 13:33, Tim Haines wrote:

Hi, Mark.

I built the head of master on all of our usual test platforms, and only found this isshttps://github.com/dyninst/dyninst/blob/e22e663ed73bfe507fcd56b72ee13043634ae4c1/instructionAPI/h/Result.hue on Power8 with gcc-7.3.1 (Power9 with gcc-7.3.1 was fine, so that's fun!).

Honestly, the PR* macros aren't the correct solution to this problem. I probably should have forced a code change before merging this, but it had already been tested by the person who submitted the PR, and it was blocking their build cycle. The correct solution is to use to use std::to_string introduced in C++11. I'll make a PR for that and re-test.

Thanks.

Â- Tim

On Tue, Aug 6, 2019 at 3:35 PM Mark W. Krentel <krentel@xxxxxxxx> wrote:

One of my cron jobs tripped an alarm for Dyninst master on powerpc
this morning.

This one is bizarre, but the last commit about the PRIx64 macros
breaks the build on some systems, depending on the exact version of
the <inttypes.h> header file.

Here is a unit test that reproduces the problem. This is a snippet
from instructionAPI/h/Result.h, near line 453.

print.cpp:

#include <sstream>
#include <string.h>
#include <inttypes.h>

int
main(int argc, char **argv)
{
  Âchar hex[20];
  Âint64_t val = 123;

  Âsnprintf(hex, 20, "%" PRIx64, val);

  Âprintf("ans = %s\n", hex);
  Âreturn 0;
}

On one power8 system with glibc-headers-2.17-157.el7.ppc64le,

$ g++ -g -O -std=c++11 print.cpp

print.cpp: In function 'int main(int, char**)':
print.cpp:11:27: error: expected ')' before 'PRIx64'
   snprintf(hex, 20, "%" PRIx64, val);
              ^~~~~~

Building Dyninst, I get the same error in Result.h near line 453.

I had to diff <inttypes.h> across systems to figure out what's going
on. The powerpc version has the following. The x86_64 version does
not have the #if on lines 44-46.

 Â44 /* The ISO C99 standard specifies that these macros must only be
 Â45  Âdefined if explicitly requested. */
 Â46 #if !defined __cplusplus || defined __STDC_FORMAT_MACROS
 Â47
 Â48 # if __WORDSIZE == 64
 Â49 # define __PRI64_PREFIX    "l"
 Â50 # define __PRIPTR_PREFIX   Â"l"
 Â51 # else
 Â52 # define __PRI64_PREFIX    "ll"
 Â53 # define __PRIPTR_PREFIX
 Â54 # endif

So, on powerpc, things like PRIx64 are either not defined or else not
defined the way you want them.

I'm not sure if this is a glibc-headers version issue, or an x86 vs
ppc issue. It seems to be more common on powerpc, but older RH 6.x
x86_64 systems also have the same problem.

Anyway, I'll let you decide the best and most portable way of handling
this. Maybe you just want to define __STDC_FORMAT_MACROS and test
that it doesn't break anything else.

Blech. My kingdom for portability!

--Mark


_______________________________________________
Dyninst-api mailing list
Dyninst-api@xxxxxxxxxxx
https://lists.cs.wisc.edu/mailman/listinfo/dyninst-api