Monorail Project: nativeclient Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 3 users
Status: Fixed
Owner: ----
Closed: Feb 2015
Cc:
Components:



Sign in to add a comment
PNaCl: va_arg does not work on nested struct types
Project Member Reported by jfb@chromium.org, Mar 18 2014 Back to list
The current version of ExpandStructRegs does not handle nested structs:
  lib/Transforms/NaCl/ExpandStructRegs.cpp

This is a known limitation, left unimplemented because it was unclear whether it was needed at the time. We now know that it is, in fact, needed because of user reports. It should therefore be implemented.
 
Project Member Comment 1 by mseaborn@chromium.org, Mar 18 2014
Which user reports are you referring to?  Is it the problem with passing nested structs as varargs arguments (noted in https://groups.google.com/d/msg/emscripten-discuss/PdMzmzmpySw/C2GJaNQ8hwEJ)?
Project Member Comment 2 by jfb@chromium.org, Mar 18 2014
Yes, it's related to varargs arguments.
Seems like the user is me.
I'm building mruby with PNaCl.
The va_arg compiling now passes with pepper_canary toolchain.
Though I get following when linking:

Value:   %.fca.0.0.extract = extractvalue %union.mrb_value %.insert, 0, 0
Value:   %.insert = insertvalue %union.mrb_value undef, %union.anon %.field, 0
LLVM ERROR: ExpandStructRegs does not handle nested structs
Value:   %.fca.0.0.extract = extractvalue %union.mrb_value %.insert, 0, 0
Value:   %.insert = insertvalue %union.mrb_value undef, %union.anon %.field, 0
LLVM ERROR: ExpandStructRegs does not handle nested structs

Project Member Comment 4 by mseaborn@chromium.org, Mar 20 2014
Here's a simple reproducer:

#include <stdarg.h>

struct S {
  int x;
  struct T {
    int x;
    char data[200];
  } nest;
};

int nest1(va_list args) {
  struct S val = va_arg(args, struct S);
  return val.x;
}


This produces:

$ pnacl-clang varargs_struct_nested.c -S -o - -O1
...
define i32 @nest1(i32* nocapture %args) #0 {
entry:
  %0 = va_arg i32* %args, %struct.S
  %.fca.0.extract = extractvalue %struct.S %0, 0
  ret i32 %.fca.0.extract
}


This is tricky to fix.  Simply extending ExpandStructRegs' existing approach to recurse into nested structs will produce a blow-up in code size in some cases.

Currently, ExpandStructRegs splits up a struct-variable into one variable per struct field.  If it did that for "char data[200]", it would create 200 separate variables.

This problem is specific to le32 targets, where Clang generates a va_arg IR instruction that returns a struct value.  On other targets, where Clang doesn't compile C va_arg() to IR va_arg but instead outputs pre-lowered code that memcpy()s the struct, the memcpy doesn't have this problem.
Project Member Comment 5 by mseaborn@chromium.org, Mar 20 2014
Summary: PNaCl: va_arg does not work on nested struct types (was: ExpandStructRegs does not handle nested structs)
Project Member Comment 6 by jfb@chromium.org, Mar 20 2014
I'm not sure I understand: if ``struct S`` had a ``char data[200]`` member wouldn't the same blowup occur? It kind of sounds like it would from your description?
Project Member Comment 7 by mseaborn@chromium.org, Mar 20 2014
Yes, if you do "va_arg(args, struct S2)", where:
  struct S2 {
    char data[200];
  };
then you get the same problem.  The problem occurs with arrays nested inside structs as well as structs nested inside structs.
Project Member Comment 8 by mseaborn@chromium.org, Apr 17 2014
I was tempted to do a quick fix to make ExpandStructRegs work on nested struct types, even if it causes a blow-up in program size on some programs, since at least it would work OK for small struct types and work inefficiently for large types.

However, I realised that there is a related problem with union types involving floats/doubles ( issue 3822 ), which could result in subtly incorrect code being generated.
Project Member Comment 9 by sehr@google.com, Jun 20 2014
Labels: -Mstone-35 Mstone-X
Project Member Comment 10 by bugdroid1@chromium.org, Dec 28 2014
The following revision refers to this bug:
  http://src.chromium.org/viewvc/native_client?view=rev&revision=14234

------------------------------------------------------------------
r14234 | jfb@chromium.org | 2014-12-28T18:58:21.996147Z

Changed paths:
   M http://src.chromium.org/viewvc/native_client/trunk/src/native_client/pnacl/COMPONENT_REVISIONS?r1=14234&r2=14233&pathrev=14234

PNaCl: Update LLVM revision in pnacl/COMPONENT_REVISIONS

This pulls in the following LLVM change:

9eecd82: (jfb@chromium.org) PNaCl: Handle nested structure types in -expand-struct-regs.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3815
TEST= PNaCl toolchain trybots
TBR=jvoung@chromium.org
NOTRY=true

Review URL: https://codereview.chromium.org/819303004
-----------------------------------------------------------------
Project Member Comment 11 by bugdroid1@chromium.org, Dec 29 2014
The following revision refers to this bug:
  http://src.chromium.org/viewvc/native_client?view=rev&revision=14236

------------------------------------------------------------------
r14236 | jfb@chromium.org | 2014-12-29T16:00:17.717700Z

Changed paths:
   M http://src.chromium.org/viewvc/native_client/trunk/src/native_client/toolchain_revisions/pnacl_newlib_raw.json?r1=14236&r2=14235&pathrev=14236
   M http://src.chromium.org/viewvc/native_client/trunk/src/native_client/toolchain_revisions/pnacl_translator.json?r1=14236&r2=14235&pathrev=14236
   M http://src.chromium.org/viewvc/native_client/trunk/src/native_client/toolchain_revisions/pnacl_newlib.json?r1=14236&r2=14235&pathrev=14236

Update revision for PNaCl r14232->r14234

Pull the following PNaCl changes into NaCl:
  r14234: (jfb@chromium.org) PNaCl: Update LLVM revision in pnacl/COMPONENT_REVISIONS
    | 9eecd82: (jfb@chromium.org) PNaCl: Handle nested structure types in -expand-struct-regs.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3815
TBR= jvoung@chromium.org, jfb@chromium.org
TEST=git try
NOTRY=true
(Please LGTM this change and tick the "commit" box)

Review URL: https://codereview.chromium.org/826183002
-----------------------------------------------------------------
Project Member Comment 12 by mseaborn@chromium.org, Feb 18 2015
Labels: LLVMIRSimplifications
Project Member Comment 13 by jfb@chromium.org, Feb 18 2015
Cc: jfb@chromium.org mseaborn@chromium.org
Labels: -Mstone-X
Owner: ----
Status: Fixed
I think this was fixed in:
  https://codereview.chromium.org/460053003/
?

Assuming so, please reopen if that's not the case.
Sign in to add a comment