[cfe-commits] [Windows] Fix PR13389 - wrong mangling of return type qualifiers

[Timur Iskhodzhanov]

See attached:
I've generated some more test cases for more coverage - all are OK
with the bug_13389_2 patch.
Also slightly re-factored the code to be ... well, a bit more readable.

Some thoughts:
I've noticed there are a few places in the code like this:
-------------
  if (!PointeeTy.hasQualifiers())
    // Lack of qualifiers is mangled as 'A'.
    Out << 'A';
-------------
which cover Pointer and Reference types.

That said, when there are no qualifiers,
a) Pointer and Reference types add 'A' (already OK)
b) built-ins shouldn't have extra 'A' (already OK)
c) the others seem to be needing 'A'.

So there are two ways to fix (c):
1) Add an extra 'A' when mangling return types (see patch; works OK)
   This roughly follows the logic of Pointer/Reference return type mangling.
2) Always add extra 'A' when mangling
  non-builtin-non-pointer-non-reference type (e.g. in mangleQualifiers).
    I'm pretty sure (2) will break something else.
    I've tried a few things (like refactoring) here and there but the
    deeper I went the more broken it was, so I've currently abandoned it.

I hope (1) is OK - at least as a temporary solution
(better than now, passes on 38 new tests, has some logic behind it).

If (1) is not OK - suggestions on improving (1) or doing (2) are welcome!

On Fri, Jul 20, 2012 at 2:00 AM, John McCall <rjmccall at apple.com> wrote:
> On Jul 19, 2012, at 2:59 PM, Chandler Carruth wrote:
>
> On Thu, Jul 19, 2012 at 2:16 PM, John McCall <rjmccall at apple.com> wrote:
>>
>> On Jul 19, 2012, at 1:39 PM, Timur Iskhodzhanov wrote:
>> >> Otherwise it seems very, very ad-hoc,
>> > I do think this is more of a general rule as the same ABCD encoding
>> > should be used to fix http://llvm.org/PR13182 too.
>> > Maybe I should merge these two fixes into one patch and extract a
>> > "mangleCVQualifiers" function? WDYT?
>> > I was planning to have this as two separate patches.
>>
>> If this is just about mangling CV qualifiers, then that sounds fine.
>> How you choose to structures the patches doesn't matter much to me.
>>
>> >> which makes me worried that
>> >> this it only works for the test cases we've run through it already.
>> > That's TDD, right? :)
>>
>> TDD is not really appropriate here.  We're not developing to vague
>> requirements, we're implementing a spec.  The Chromium build is not
>> going to give us complete, or even near-complete, coverage of MSVC's
>> mangling scheme.  Fixing just enough to get Chromium to build is not
>> going to leave us with a working mangler;  it is going to leave us with
>> a mangler that needs to be fixed again, and again, and again, every
>> time we push a new project through it (or, for that matter, a new version
>> of Chromium).
>>
>> You need to be running experiments and trying to look for unifying
>> principles behind MSVC's manglings.  Like, you're mangling qualifiers,
>> but not when it's a pointer, reference (?!), or builtin type that's
>> qualified?
>> Something like that?  That's a rather complicated list of special cases.
>> Now, it's possible that there is no fundamental rule here, and that MSVC's
>> mangler has logic just as convoluted in it;  if so, fine, the spec's the
>> spec.
>> But please do the investigation, looking for corner cases that illustrate
>> the general rule, and leave comments and test cases behind you that
>> tell us what you've done.
>
>
> FWIW, I agree w/ John, but I actually think TDD can get us here, we just
> need to be a bit more creative with the tests.
>
> Imagine you start w/ a build failure in Chromium. You realize it has to do
> with qualifiers, and you have the first test case.
>
> Now, generate a *comprehensive* set of test cases for how qualifiers impact
> mangling by varying all of the bits John mentions. You can do this
> programmatically, and test all of them, and then you'll have test failures
> which specifically indicate how best to architect the mangling for that
> entity in order to not just handle a few cases, but to handle the system in
> general.
>
> Anyways, what ever the path to getting there is, getting this fundamentally
> correct and tested across the edge cases and axes in which it can break is
> the important result. =]
>
>
> Agreed!
>
> John.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug_13389_3.patch
Type: application/octet-stream
Size: 4211 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120720/69b1c87d/attachment.obj>