Hi Alexey, On Tue, Mar 26, 2019 at 4:26 PM Alexey Ivanov <[email protected]> wrote:
> Hi Thomas, > > On 26/03/2019 14:41, Thomas Stüfe wrote: > > Hi Alexey, > > > > On Tue, Mar 26, 2019 at 2:36 PM Alexey Ivanov <[email protected]> > wrote: > >> Hi Thomas, >> >> Looks good, I'm not a reviewer though. >> Could I also ask you to push this changeset to jdk/client [1] instead of >> jdk/jdk? >> >> A small question: two prototypes are changed in debug_trace.h but only >> one is updated in debug_trace.c. Is it because the functions matching >> the second prototype have already been updated with JNICALL? >> >> > Not really sure I understand the question. There is no real 1:1 > relationship between my change in .h and .c: > > > I mean there are two prototypes in the header: > 1. typedef void (JNICALL *DTRACE_OUTPUT_CALLBACK)(const char * msg); > 2. typedef void (JNICALL *DTRACE_PRINT_CALLBACK)(const char * file, int > line, int argc, const char * fmt, va_list arglist); > > 1. In C, only the first one is updated: > static void JNICALL DTrace_PrintStdErr(const char *msg); > which corresponds to the first case. This shouldn't have caused build > failure because the declarations in .h and .c matched. > > 2. I believe DTrace_VPrintln and DTrace_VPrint are the callbacks for the > 2nd prototype of DTRACE_PRINT_CALLBACK. They're received JNICALL under > JDK-8214120 but the header hasn't been updated. This caused the build > failure. > > I cannot find any place where function pointers to DTrace_VPrintln or DTrace_VPrint are used. They only ever are called directly. > The prevent the build failure, it would be enough to update the 2nd > declaration in the header. (Or alternatively, drop JNICALL from > DTrace_VPrintln and DTrace_VPrint in .c file.) > For consistency, you're updating the 1st declaration as well. > > > DTRACE_PRINTxx macros call _DTrace_Template() which expands to > DTrace_PrintFunction(). First argument is pointer to DTrace_VPrint(), which > is a function decorated with JNICALL. Hence the build error - the type of > the first argument of DTrace_PrintFunction is DTRACE_PRINT_CALLBACK, which > is a fkt pointer *without* JNICALL. > > So one way to fix this was to correct the DTRACE_PRINT_CALLBACK type to be > a fkt pointer pointing to a JNICALL decorated function. > > That means that I also needed to fix all functions whose pointers are > being passed around as DTRACE_PRINT_CALLBACK. But there only was one I saw, > DTrace_PrintStdErr(). > > > From the above, it looks this isn't necessary as DTrace_PrintStdErr() is > of type DTRACE_OUTPUT_CALLBACK. > > However, I think it's better to have both DTRACE_OUTPUT_CALLBACK and > DTRACE_PRINT_CALLBACK consistent: either with JNICALL or without. > > > Looks fine to me! > > Great, I pushed. Thanks for reviewing! --Thomas > I'll push to jdk/client. Will that be transported to jdk/jdk automatically? > > > Yes, it will. > Although I'm not sure how often they're synced. > > Regards, > Alexey > > > Cheers, Thomas > > > > > >> Regards, >> Alexey >> >> [1] http://hg.openjdk.java.net/jdk/client/ >> >> On 25/03/2019 13:09, Thomas Stüfe wrote: >> > Hi all, >> > >> > Issue: https://bugs.openjdk.java.net/browse/JDK-8221405 >> > cr: >> > >> http://cr.openjdk.java.net/~stuefe/webrevs/8221405-windows32-awt-buildfixes/webrev.00/webrev/ >> > >> > On 32bit windows, awt build did bitrot. Dtrace print callbacks need to >> > be declared with __stdcall. No other platform cares. >> > >> > Thanks, Thomas >> >> >
