Well thanks for all that. I'll certainly take another look and report back before committing anything. Giving a compiler family indigestion is certainly to be avoided.
Cliff On Tue, Jan 28, 2014 at 11:29 AM, Fraser Adams <[email protected]> wrote: > 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) <[email protected]> >> 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) > >
