Re: Serious bug in auto-import

[Charles Wilson <cwilson at ece dot gatech dot edu>]

Paul Sokolovsky wrote:

> Hello Charles,
> 
> Charles Wilson wrote on Sunday, September 09, 2001:
> 
> []
> 
> CW> Yep.  Tested.  ld with Paul's patch accurately detects, prints out a
> CW> warning message, and exits-with-error whenever "direct addressing" is
> 
>     Yep, if I didn't confused it and used right English term (I
> learned that long ago in Russian). Well, for sure it must right. In
> Intel's terms, it is "direct addressing with [non-zero] displacement"
> (vs for example with indirect addressing when [part of] address is held
> in register). I used term "offset" instead of "displacement" though
> (Intel used to mark offset within 8086 64k segements).


Your wording:

aggregate 'st' is referenced in direct addressing mode with constant 
offset - auto-import failed. Workarounds: a) mark the symbol with 
__declspec(dllimport); b) use auxiliary variable

Seems good to me.  I think both options should be mentioned (briefly, as 
you have done), because recommending only __declspec(dllimport) leads us 
*right* back to the nasty "on cygwin, if static...else if building 
dll...else if linking to dll..." garbage.  (more on this below).

> 
> CW> Now, with structs, it's a little tricky.  Assume the following definition:
> CW> typedef struct {
> CW>    int a;
> CW>    int b;
> CW> } ST_;
> 
> CW> This is bad:
> CW> extern ST_ st;
> CW>    st.a = ...
> 
>        Well, in fact *this* exact code is good. It's possible to
> access the very first member of struct ;-)


Yeah, I forgot that my test program had the .a and .b references 
transposed (next time, I should cut-n-paste, not retype by hand)


> 
> CW> This is also bad (but is an analogous form to the "array" version that
> CW> works)...
> CW> extern ST_ st;
> CW> ST_ p;
> CW>    p = st;
> CW>    p.a = ...
> 
>        Here's the trick: it should be working in general (get the
> address of st, memcpy()  it), but gcc optimizes 8-byte copy, into two
> consecutive stores, hence problem.
> 
>        Nevermind. It will tell when it's bad, and what is needed is to
> know how to workaround such case.


exactly my point.  A brief explanation in the error message (as you 
have), coupled with more explanation in the .info file.  I'll try to 
work up a patch for ld.texinfo tonight.

>     As you see, I tried to hammer in howto into error message ;-)
> (yes, it hardly will make sense for outsider). I think, there should
> be standard recommendation: mark offending symbol with
> __declspec(dllimport) (and error message should state only this
> alternative).


I disagree.  While the auto-export option means that you don't need to 
specify __declspec(dllexport), you still need to distinguish (on cygwin) 
between "I'm building client code that I will link to the DLL" vs. "I'm 
building client code that will link to the static lib"

This means that you can't just decide at link time to say "ld 
-Bdynamic"(default gcc behavior) or "ld -Bstatic"(gcc -static).  You 
ALSO have to -DFOO_DLL_LINK or -UFOO_DLL_LINK (conversely, 
-DFOO_STATIC_LINK and -UFOO_STATIC_LINK) when compiling, so that the 
decoration is #defined appropriately.  But, you only have to do that if 
your package accesses "aggregates" from its dependencies, if it uses 
direct accessing with constant offset....

Blech.  This gets really messy.  Joe Schmoe ought to be able to build a 
client package without knowing those internal details of the library. 
And convincing the maintainers of that client code to uglify their 
headers with all those "extern STUPID_CYGWIN_DECORATOR int foo" is a 
real challenge -- trust me, I've tried.

Therefore, I *much* prefer recommending the second of your alternatives 
-- I wasn't aware that volatile would "fix" it for us.  (And you can use 
volatile for ALL THREE cases on cygwin; static, DLL(build), and 
DLL(linkto) -- I think. I will test this tonight.).  One question, 
though:  you say:

 > /* This stupid workaround is for win32 - do not "optimize" */
 > volatile int *parr = arr;

Does your comment mean
   "Future programmer editing this client code, do not replace 
references to 'parr' with 'arr'."
or
   "When compiling this source code, do not specify -O{n}"
or both?

Will -O2 override an explicit "volatile" declaration?  I didn't think it 
would do that...I will verify.

Therefore, ld's error message should at least HINT at the "use a local 
variable" possibility.  Your original wording does that.  Then, more 
detailed stuff would go in the .info file (I'll base my ld.texinfo patch 
on your explanation below).

> If you care, in ld you can add more elaborated
> discussion, along the lines of:
> 
> =====
> The problem happens when some (sub)expression accessing address
> ultimately given by the sum of two constants. The solution is to make
> it use one variable instead. In case of array, we have two
> possibilities: a) make indexee (array) variable, b) make index
> variable, i.e.:
> 
> extent type extern_array[];
> 
> extern_array[1] => { volatile type *t=extern_array; t[1] }
> 
> or
> 
> extern_array[1] => { volatile int t=1; extern_array[t] }
> 
> For struct, the only option is to make struct itself variable:
> 
> extent struct s extern_struct;
> 
> extern_struct.field => { volatile struct s *t=extern_struct; t->field }
> 
> Which workaround (__declspec(dllimport) marking or rewriting rules
> above) you should use? Obviously, __declspec(dllimport) marking,
> unless you have the aim not to introduce system-dependent features to
> your code (but note that alternative is to introduce obscure
> constructs into your code, so you will need to comment why you did
> this anyway). In short, consider typical real-world usage of both
> methods and decide yourself:
> 
> Original :
> --foo.h
> extern int arr[];
> --foo.c
> #include "foo.h"
> main()
> {
>   printf(arr[1]);
> }
> --
> 
> 1:
> --foo.h
> /* Note: auto-export is assumed (no __declspec(dllexport)) */
> #if defined(_WIN32) && !defined(FOO_BUILD)
> #define FOO_IMPORT __declspec(dllimport)
> #else
> #define FOO_IMPORT
> #endif
> extern FOO_IMPORT int arr[];
> --foo.c
> #include "foo.h"
> main()
> {
>   printf(arr[1]);
> }
> --
> 
> 2:
> --foo.h
> extern int arr[];
> --foo.c
> #include "foo.h"
> main()
> {
>   /* This stupid workaround is for win32 - do not "optimize" */
>   volatile int *parr = arr;
>   printf(parr[1]);
> }
> --
> 
> 
> =====
> 
> []
> 
> CW> Unless of course:
> 
> 
>>>THEN we can think about a "real" fix for this array-index problem (if
>>>possible; it may not be). :-)
>>>
> 
>    Well, my guess is that occurrences of the issue will be so rare,
> that "real fix" will never pay for itself...


Actually, given the "volatile" fix, I like it.  This usage seems to be 
pretty uncommon, so the bug (and error message) will rarely be hit. 
Recommending "volatile" markers means when only need to #ifdef on 
__CYGWIN__ , not a bunch of FOO_STATIC/FOO_DLL_BUILD/FOO_DLL_LINKTO 
variables.

Getting rid of THAT garbage was my primary goal in advocating your 
auto-import stuff in the first place!

So: we have a rarely triggered bug.  Your patch warns the user and 
prevents buggy code from being generated. My forthcoming documentation 
patch (based on your stuff, above) will explain how a user hit by this 
bug can work around it.  And we still don't need 
FOO_STATIC/FOO_DLL_BUILD/etc.

I'm happy.  Now, about that memory leak.... :-)

--Chuck