Re: gettext patches for cygwin #4: accessing fields of exported structs/arrays

[Charles Wilson]

[Top posting the summary paragraph from the bottom of this long and 
rambling message]

-----
And, since you're the maintainer, you get to make the call. <g>

I think, with regards to THIS patch, we're in agreement: it shouldn't be 
applied, and I/cygwin should rely on the DLL_VARIABLE decorations you 
have added, instead of my 'use a function instead' changes. 
(Notwithstanding that you're already using the 'use a function instead' 
solution for all platforms for po_gram_error*, in CVS)


But with regards to the C++ workaround, it only solves the problem

[of the .rdata/long_options[] issue and my patch #1, NOT the 
po_gram_error*/plural_tables[] auto-import issue and my patch #4]

if the variables exported by the DLL are all properly declspec'ed; 
auto-import hides the compile-time gcc error AND causes both gcc and g++ 
to generate broken apps in these circumstances.
-----

Bruno Haible wrote:
> Charles Wilson wrote:
> > If that is true, then there is a bug in gcc (and not simply a platform
> > insufficiency -- "you can't do X on platform Y").  If it can be done by
> > g++ then it should also be done by gcc.
>
> If you want to work on pushing this feature from g++ into gcc, I can
> explain to you how g++ does it. 

[snip explanation; thanks]

> The C compiler notices that &option_x is not of the type of constant
> expressions allowed in initializers (linker constants or addresses or
> differences between two addresses), and gives an error.

Actually, it doesn't give a compile error: I can compile and link, and 
get a "valid" executable. Only when the executable is launched, I get a 
popup window complaining that the app couldn't be initialized.  This is 
because the relocation machinery is TRYING to put the appropriate stuff 
into long_options[], but can't because long_options is in .rdata, which 
is loaded into non-writable memory.

Plus, by your analysis, I wouldn't be able to fix it -- and I *can* fix 
it -- simply by making long_options[] non-const.

The real key is that, if you're right, g++ is smart enough to look at 
long_options and say "waitaminute, I've got to initialize this thing at 
runtime so I'd better ignore the const specifier" -- and voila, the 
struct DOESN'T go into .rdata! (thus, any g++ initialization and/or 
relocation machinery is allowed to write to the struct's storage, and 
everybody's happy).

Let's test:

----- bar.c ----
int var = 14;
int foo(int a)
{
  return a * a;
}
-----------------

----- foo.c -----
#include <stdio.h>

extern int foo(int a);
extern int var;
int * const b = &var;

int main(int argc, char** argv)
{
  printf("%d\n", *b);

  *b = 2;
  printf("%d\n", *b);

  return 0;
}
-----------------

gcc -shared -o cygbar.dll -Wl,--out-implib=libbar.dll.a bar.c
gcc -o foo.o -c foo.c
gcc -o foo.exe foo.o -L. -lbar

./foo.exe fails to initialize.

g++ -o foo.o -c foo.c
g++ -o foo.exe foo.o -L. -lbar

./foo.exe fails to initialize.

Note that I get the same results using gcc and g++ (3.4.4-cygwin).

Now, removing the 'const' from the declaration of 'b' in foo.c, and 
recompiling, gives an app that works.  (plus, checking foo.o with nm 
shows that in the former case, _b is in .rdata, but in the latter case, 
it is in .data).

Hmm.  Can't initialize a const pointer with a relocated address.  Maybe 
g++ is "smart" in that it treats a struct -- even a struct with static 
initializers -- like a class.  That is, it "knows" to create a 
constructor, "knows" that the constructor must be called at runtime, and 
therefore "knows" to ignore the 'const' in that it won't stick the 
struct into .rdata.

Okay...

----- foo2.c -----
#include <stdio.h>

extern int foo(int a);
extern int var;

struct dummy_ {
  char* name;
  int*  num;
};
typedef struct dummy_ dummy;

int a = 7;

const dummy baz[] = {
    { "one", &a },
    { "two", &var }
};

int main(int argc, char** argv)
{
  *(baz[0].num) = 1;
  *(baz[1].num) = 3;

  printf("%s: %d\n", baz[0].name, *(baz[0].num) );
  printf("%s: %d\n", baz[1].name, *(baz[1].num) );

  return 0;
}
-----------------


gcc -o foo2.o -c foo2.c
gcc -o foo2.exe foo2.o -L. -lbar

... app fails to initialize ...

g++ -o foo2.o -c foo2.c
g++ -o foo2.exe foo2.o -L. -lbar

WAIT!

Once again, foo2.exe can't be initialized.  Even using g++, I still get 
an app that fails to initialize (_baz is in .rdata).

If I replace { "two", &var } with { "two", &a }, everything is fine 
(_baz is in .rdata, but that's okay: &a doesn't need to be relocated).

If I remove the const from the baz declaration, everything is fine (_baz 
is in .data).


Okay, one last try: maybe it's the auto-import stuff that's interacting 
badly.  What if we explicitly use the declspec -- will that be enough of 
a hint to g++ to Do The Right Thing?

----  bar2.c ----
__declspec(dllexport) int var = 14;

int foo(int a)
{
  return a * a;
}
-----------------

foo3.c == same as foo.c above, only declare 'var' as
   extern __declspec(dllimport) int var;

gcc -shared -o cygbar2.dll -Wl,--out-implib=libbar2.dll.a bar2.c
gcc -o foo3.o -c foo3.c

A-HA! THERE's the compile-time error you've been seeing and I haven't! 
It's because I've been relying on auto-import, and you've been using 
declspec!  BUT, the compile-time error only shows up with gcc...

g++ -o foo3.o -c foo3.c
g++ -o foo3.exe foo3.o -L. -lbar2

HEY! that works...

Okay, but what about structs?

foo4.c == same as foo2.c above, only declare 'var' as
   extern __declspec(dllimport) int var;

gcc -o foo4.o -c foo4.c

Whaddaya know, more compile-time errors!

g++ -o foo4.o -c foo4.c
g++ -o foo4.exe foo4.o -L. -lbar2

It works!


Okay, it appears that the g++ workaround ONLY works if the imported 
variable is declspec'ed.  It does NOT help if you're using auto-import.

Now, everything below is under the assumption that auto-import is being 
used...


> Such "constructor" functions are also supported by gcc in C mode, through
> __attribute__((__constructor__)). Therefore there is no fundamental
> reason why gcc could not do the trick that g++ does.

Woah.  constructors for C code.  Well, I guess that's not so different 
than the runtime-pseudo-reloc code in the mingw/cygwin runtime 
libraries...and more on THAT subject later!

> > By introducing C++ compilation, you've wandered into the
> > hell that is C++ ABI variation.
>
> If you push the above g++ technique into gcc, I don't see any ABI
> problems appear.

[[auto-import, not declspec]]
True -- C doesn't name-mangle.  So if gcc were to create these little 
'constructors' (and automagically ignore 'const' by NOT putting the 
affected variable into .rdata) when appropriate...big win.  Problem 
solved.  Actually, as described above it's probably enough if gcc were 
'smart' enough to realize when particular variables need to be 
initialized with runtime-relocation addresses, and make sure that the 
variable is NOT put into .rdata even if declared const.

The "constructor" stuff you're talking about is already being handled -- 
it just fails when the variable to be adjusted with the __imp__foo() 
address is non-writable.  That's why everything is peachy if I simply 
remove the 'const' from the long_options[] declaration.

Now, if only I had a spare week (or seven) to grok gcc's source code, 
and make gcc smart enough to know when NOT to put a "const" variable 
into .rdata!  It's been almost five years since I last worked on the gcc 
code, and then I was just bird-dogging someone else's efforts.  Well, 
it's definitely a tractable problem and it'll go into my list for 
rainy-day programming projects.




but...all of the preceding is really a discussion about .rdata, 
initialization, etc.  (Technically, belongs in patch #1 thread)

It was my fault for commingling these two issues (1) .rdata/const stuff, 
(2) direct access to 'complex' exported variables.  THIS thread *should* 
have been about (2); the patch #1 thread *should* have been about (1). 
Silly me, I had to bring a discussion of (1) into THIS thread. I'm sorry 
about that.  I'll try to keep the two issues separate in the discussion 
below.

> There are four reasons why I chose the C++ wrapper files approach

Again, the C++ wrapper file thing is a workaround for the .rdata issue. 
 That issue is separate from the one I was trying to address with THIS 
patch: the direct access to fields of an array of structs (or, for that 
matter, to fields of exported single structs, or elements of an exported 
array).

The ld documentation lists four ways around this problem:

(1) -enable-runtime-pseudo-reloc linker switch
This is the default for ld as of July 2005; the current official cygwin 
binutils has this change.  So, once again, gettext-0.14.5 might "just 
work" with respect to this issue without any changes...my patches are 
behind the times.

(2) play games with volatility and constness; use explicit pointers, 
etc.  This is truly ugly stuff.

(3) declspec(__dll[im|ex]port) the enclosing variable.  This is what you 
did.

(4) use a functional interface instead of trying to access variables 
directly.  This is what I did.

> instead of the solution that you are proposing:
>
> 1) The existing code is ANSI C. According to the principle "fix the
>    broken code, not the correct code", it is the wrong place to
>    modify po-lex.[h|c] because of problem that is in Woe32.

Ok, but my point was that (as of 0.14.5) you WERE 'fixing the correct 
code' in po-lex for certain platforms, with regards to 
po_gram_error[_at_line]:

#if (some compilers)
   use 'valid ANSI C code' in a macro -- which had the effect of 
inducing clients to directly accesss blah blah blah
#else (other compilers)
   use function calls (also 'valid ANSI C code').

That was *existing* code in 0.14.5, not something I added.  I simply 
added cygwin to the list of 'other compilers' so that the function calls 
you provided were used on cygwin.

In current CVS, you ripped out the macros entirely; now everybody uses 
the function calls to do po_gram_error[_at_line] stuff.

So, going forward, this particular part of my patch is a nullity, since 
it's really already in effect in CVS gettext.



Now, the other part of this patch, concerning plural_table[j].foo 
accesses -- I simply followed the precedent you set with the 
po_gram_error accesses -- that is, option 4 from the ld documentation: 
use a function.  The weakness I saw in THIS part of this patch even 
before I sent it in, was that it was unconditional: I changed the code 
so that ALL platforms would use the new accessor function instead of 
accessing plural_table[] directly.  This is the kind of sweeping change 
that maintainers of cross-platform software usually get exercised about...

But by recent CVS history w.r.t. po_gram_error, you've now 
*strengthened* my case: with po_gram_error, YOU have now forced all 
platforms to do po_gram_error accesses via a function call -- why not 
plural_table, as well?  'Tis also 'valid ANSI C code'...



However, declspec'ing the variable (option 3 from the ld docu) should 
work too -- and obviously it does in your testing.  *I* prefer using the 
auto-import features of gcc on cygwin and mingw (when they work); I find 
the whole

#if woe32-related && !building_static
#if building_dll
#define SOMETHING declspec(__dllexport)
#else
#define SOMETHING declspec(__dllimport)
#endif
#else
#define SOMETHING
#endif

thing ugly (plus, you have to go decorate all the variables with 
SOMETHING).  Have you SEEN how many vars are exported by the readline 
DLL? Man was I glad when I no longer had to patch each new readline 
release at 400 locations (and my patches never applied cleanly to the 
new version; too much thrash).

On the other hand, the declspec mayhem is required if you want to enable 
building DLLs on windows with non-gcc...so if you have already done the 
work to ensure the machinery is there, why am I complaining...we can 
just use that on mingw/cygwin -- which nicely avoids the case where 
'auto-import' **used to** break ('complex' exported variables).

Plus, your C++ workaround only works when the target variable is 
declspec'ed, so that's where these two issues overlap: in order for you 
to use C++ to work around the .rdata/long_options[] issue, you need the 
DLL_VARIABLE machinery anyway.



The auto-import related parts of this patch (both po_gram_error-related 
and plural_table[]-related) were motivated by the auto-import breakage 
for "complex" variables, back before --enable-runtime-pseudo-reloc was 
available.  And, once again, I've just been forward-porting the same 
patch with each new gettext release for cygwin.

If I were had first encounted this problem last January, I would've 
probably just added --enable-runtime-pseudo-reloc to the link flags, 
instead.  And obviously, if I had first tried to compile gettext on 
cygwin in the last four months, once --e-r-p-r became the default, I 
never would have seen the problem to begin with.

However, declspec'ing the offending vars is a perfectly good solution. 
I think the bigger disagreement is actually related to the .rdata issue, 
and the 'scaffolding' of C++ wrappers etc -- which is really fodder for 
the patch #1 thread.

> 2) My changes are conceptually easy, i.e. they don't require insight
>    in the program. Testing which files give an error when compiled in
>    C mode is something a porting person can do without understanding
>    the program.

Again, .rdata related and should be over in the patch #1 thread.  But 
I'm not really sure what you're getting at, here.  With the const 
long_options[] stuff, I never got an error at compile time, nor link 
time.  It was always runtime.  And THAT's the issue your C++ workaround 
solves [?], and which my FUNKY_CONST_MACRO also solves, and which your 
porting person is going to have to solve in the new msgfoo program, or 
in the modified gettext program (if somebody added a var flag to its 
long_options).

Your porting person is still going to have to say "hey, msgfoo.exe gives 
me this 'failed to initialize' popup.  How do I fix it?"

"I know! Compile msgfoo.c with g++ instead of gcc."  (As if THAT were 
obvious).  OTOH, "I know! Replace 'const' with FUNKY_CONST_MACRO in the 
declaration of long_options" is non-obvious, too.

My point is, an ignorant porting person is NOT going to see either 
solution as self-evident, and they'll STILL need to execute the app to 
see if this problem persists/exists.   Porting persons should not be 
ignorant. :-)

> 3) In the process of making releases, Woe32 porting comes late before
>    a release. With your approach, you have to expect that it is needed to
>    turn a variable into a function, and other kinds of API changes.
>    I hate to make such changes right before a release, because it means
>    I have to restart testing on the other platforms.

That's certainly understandable (and here we're back discussing 
po_gram_error and plural_table, the 'complex var exported from DLL' issue).

> 4) Reliability: With my approach, I know that things work if they compile.
>    Whereas your approach requires testing of the built executables in
>    order to know whether there are still relocation problems remaining.

(okay, back to the .rdata/.const issue)

I disagree. I think you still need to check the executable, because 
that's the only time the error occurs.  It doesn't occur at compile or 
link time, even when using the C compiler -- UNLESS you are sure that 
you're not auto-importing any variables.  Maybe we need to specify 
--disable-auto-import, because auto-importing in these circumstances 
will lead to broken app, even if using g++.

--disable-auto-import shouldn't hurt, IF all exported vars from the DLL 
are properly decorated with declspec's.

I don't see that using the g++ compiler would magically transform a 
runtime error into a compile- or link-time error; and in fact, it 
doesn't. It's using auto-import that turns a compile-time error into a 
runtime error!

It's only because you trust that your g++ workaround fixes the problem, 
that you feel comfortable in saying "I don't need to check runtime 
behavior; I know g++ is doing the right thing."

OTOH, not declaring long_options[] const on affected platforms 
definitely Does The Right Thing (and that won't change: nobody is going 
to make gcc put explicitly non-const vars into .rdata!)

IOW, since the error is observable only at runtime [[ IF auto-import is 
or *might be* used ]], you can only *verify* that it is fixed, using a 
runtime test: try to run the app.

> >   But still...I wonder if it is possible that, at some time in the
> > future, g++ will start generating code similar to that currently
> > generated by gcc, given identical input.  At that hypothetical point,
> > your solution is back to square one.
>
> You mean, gcc will start generating code that is as good as g++ now?

(.rdata issue)
No, I mean the opposite: that g++ suddenly STOPS solving your problem.

Your formulation: gcc starts 'working', would be wonderful, obviously:

> At that point, it will be easy to remove the "scaffolding" - as you call
> it - from the Makefiles. And I'll have no changes to make to the actual
> source code.

(.rdata issue)
Well, yeah.  And you WOULD do this, because the scaffolding is an 
eyesore -- if gcc started to Do The Right Thing, you'd jump at the 
chance to eliminate the workaround.

> Whereas with your approach, I would have to turn back
> indirect-access functions into variables.

(now we're talking about the po_gram_error and plural_table[] stuff):

But this assumes that you would do so: IMO indirect access functions are 
not an eyesore; in fact, exposing variables from a shared library is a 
flaw from a data-encapsulation POV. But this is one of those 
eye-of-the-beholder areas, where opinion dominates.

And, since you're the maintainer, you get to make the call. <g>

I think, with regards to THIS patch, we're in agreement: it shouldn't be 
applied, and I/cygwin should rely on the DLL_VARIABLE decorations you 
have added, instead of my 'use a function instead' changes. 
(Notwithstanding that you're already using the 'use a function instead' 
solution for all platforms for po_gram_error*, in CVS)

But with regards to the C++ workaround, it only solves the problem if 
the variables exported by the DLL are all properly declspec'ed; 
auto-import hides the compile-time gcc error AND causes both gcc and g++ 
to generate broken apps in these circumstances.
--
Chuck

>