Hi, me again Cliff.
I've only had time to recheck this against my test case, which is representative of what I see for real.

Doing:

int pn_data_vfill2(const char *fmt, va_list ap)
{
    // Process the PROPERTIES constant - this seems OK
    uint64_t prop = va_arg(ap, uint64_t);
printf("prop = %llu\n", prop);

    {
        pn_bytes_t bytes = va_arg(ap, pn_bytes_t);
printf("pn_data_vfill z, bytes.size = %zu, bytes.start = %s\n", bytes.size, bytes.start);
    }

    // Process the char* returned by pn_string_get()
    {
        char *start = va_arg(ap, char *);
        size_t size = strlen(start);
printf("pn_data_vfill size = %zu\n", size);
printf("pn_data_vfill string = %s\n", start);
    }

    return 0;
}

E.g. the both passing and retrieving structs approach of your second approach actually doesn't even compile for me with LLVM le32, I get

error:
      cannot compile this aggregate va_arg expression yet
        pn_bytes_t bytes = va_arg(ap, pn_bytes_t);
                           ^~~~~~~~~~~~~~~~~~~~~~
/home/fadams/emscripten/system/include/libc/stdarg.h:15:25: note: expanded from
      macro 'va_arg'
#define va_arg(v,l)     __builtin_va_arg(v,l)
                        ^~~~~~~~~~~~~~~~~~~~~
1 error generated.
ERROR    root: compiler frontend failed to generate LLVM bitcode, halting
make[2]: *** [CMakeFiles/test_varargs.dir/test_varargs.o] Error 1
make[1]: *** [CMakeFiles/test_varargs.dir/all] Error 2
make: *** [all] Error 2

Though it *does* work if I do "EMCC_LLVM_TARGET=i386-pc-linux-gnu make"


So in a nutshell:
1) if I fudge the LLVM front end pure struct, put struct retrieve discrete and pure discrete values all work correctly.

2) with the standard emscripten le32 LLVM front end (which is what is recommended)
* pure struct (as above) fails to compile
* put struct, retrieve discrete (as with the original Proton code) gives the wrong results - that messed with my head :-) * pure discrete values (as with your pn_string_size(msg->user_id), pn_string_get(msg->user_id),) patch works correctly.


So I'd *really* like it if you could go down the path of your original patch 'cause that would fix what you've been seeing and let me compile the JavaScript stuff without needing the environment tweaked (which is an accident waiting to happen). I hope that you are agreeable to that?

I think that it would probably be worth putting a comment in the call to pn_data_fill and within the pn_data_vfill() 'z' case body to document that it's safer to pass individual entries to va_arg to avoid upsetting some compilers - it's not entirely an *obvious* thing really :-D

Cheers,
Frase



On 27/01/14 21:21, Cliff Jansen wrote:
Thanks for the Javascript related info.

Fraser: can you test if the review board patch (with the struct "in
and out" strategy) works in your case with the unhacked llvm setup?
If that works then I'll go ahead and check it in.

If it fails, please try the first patch.  If that works, we will just
have to conclude that compilers have trouble with stucts in this case
and fall back to passing the two basic types.  That should be safe to
work in the greatest number of cases.

Many thanks.

On Mon, Jan 27, 2014 at 1:13 PM, Fraser Adams (JIRA) <j...@apache.org> wrote:
     [ 
https://issues.apache.org/jira/browse/PROTON-488?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13883314#comment-13883314
 ]

Fraser Adams commented on PROTON-488:
-------------------------------------

Oh to be clear
either "struct in and out", or "separate size_t and char* in and out"

what I meant in my previous comment was that the separate size_t and char* in and out is what LLVM 
le32 is happiest with, the structs seem to confuse it, I hadn't realised that you'd changed your 
original patch to now do "all 'z' encodings to be passed as a single pn_bytes_t struct and 
"retrieved" as a single pn_bytes_t struct" - although this arguably looks neater I'd 
definitely prefer your first approach.

Windows 7 64-bit VS2010 qpid-proton Crash on Startup with Send / Recv 
Application
---------------------------------------------------------------------------------

                 Key: PROTON-488
                 URL: https://issues.apache.org/jira/browse/PROTON-488
             Project: Qpid Proton
          Issue Type: Bug
          Components: proton-c
    Affects Versions: 0.6
         Environment: Windows 7 64-bit VS 2010
            Reporter: Frank Quinn
            Assignee: Cliff Jansen
            Priority: Critical
         Attachments: PROTON-488-0.patch, qpid-proton-win64-send-crash.png


Steps to recreate:
1. Grab latest 0.6 tarball
2. Start up Visual Studio x64 Win64 Command Prompt (2010) and run "cmake ." to 
generate the visual studio files
3. Open Up the newly created Proton.sln in VS2010, right click on qpid-proton 
and add the path to python to executable directories
4. In the configuration manager, select qpid-proton and select active configuration to be 
Debug, then select "Add" to add x64 support, copying win32 configuration in the 
process.
5. Select qpid-proton properties and remove the hard coded /machine:X86 extra 
command lines in Linker -> Command Line (MACHINE:X64 should already be in the 
command line above so no need to add here)
6. Right click on qpid-proton and select build
Repeat steps 3-6 for send / recv applications. When you run recv, then run 
send, you'll get a crash with the (soon to be attached) trace.
Cheers,
Frank


--
This message was sent by Atlassian JIRA
(v6.1.5#6160)

Reply via email to