Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge 2.076.1 front-end and stdlibs #2362

Merged
merged 18 commits into from
Oct 30, 2017
Merged

Merge 2.076.1 front-end and stdlibs #2362

merged 18 commits into from
Oct 30, 2017

Conversation

kinke
Copy link
Member

@kinke kinke commented Oct 7, 2017

No description provided.

Biggest changes:

* idgen.d has been replaced by a CTFE implementation, id.d.
  I manually added a C++ header declaring the symbols we need and
  adapted (simplified) the CMake script accordingly.
* More semantic() methods have been extracted as visitors; most notably
  for expressions.
@dnadlinger
Copy link
Member

That was quick - you beat me to it. ;)

@kinke
Copy link
Member Author

kinke commented Oct 7, 2017

Oh sorry, hope you didn't invest too much time into it [I committed the submodules yesterday, so I hoped it'd be clear I'm working on it].
Thankfully looking better than 2.075, mainly just a few dmd-testsuite issues. Working on getting the unittests green first.

@kinke
Copy link
Member Author

kinke commented Oct 7, 2017

Btw, apparently there are no new -betterC tests (at least no breaking ones ;)), but people certainly assume LDC to feature similar semantics, incl. calling the C assert function etc.

@rainers
Copy link
Contributor

rainers commented Oct 7, 2017

Great work!
Option -gf for testpdb should be identical with -g for LDC, so mapping it accordingly in ldmd should be correct.

@kinke
Copy link
Member Author

kinke commented Oct 7, 2017

No new Phobos release without regressions for 64-bit reals: dlang/phobos#5762

Thanks Rainer, I was wondering whether I shouldn't accept -gf or just ignore it.

@PetarKirov
Copy link
Contributor

Btw, apparently there are no new -betterC tests

There's some in https://github.com/ldc-developers/dmd-testsuite/blob/ldc-merge-2.076/runnable/cassert.d, https://github.com/ldc-developers/dmd-testsuite/blob/ldc-merge-2.076/compilable/betterCarray.d and perhaps https://github.com/ldc-developers/dmd-testsuite/blob/ldc-merge-2.076/compilable/betterCswitch.d (don't know why the betterC switch is not used there?), though far from anything extensive. (nm and size would have made for more interesting tests, IMO.)
Probably the biggest change so far is that druntime is not linked in.
The test cases should at least guarantee that if assertions are used you can still successfully link without druntime.

I hope I didn't miss anything - fixing these bugs is super-tedious.
* druntime: Adapt test/shared to vanished osmodel.mak
* LDMD: Accept multiple `-fPIC`. It translates to
  `-relocation-model=pic`; that option is provided by LLVM and only
  allowed once in the command line.
  That issue is new for 2.076, as druntime's test/common.mak defaults to
  PIC now.
@kinke
Copy link
Member Author

kinke commented Oct 7, 2017

Semaphore, Circle, Travis: only runnable/sdtor failing. Wow. :)
On my Win64 box and for AppVeyor (both Win32 and Win64), there's an additional runnable/testpdb failure (MSVC-specific, no debug info found for object.Object).

@kinke
Copy link
Member Author

kinke commented Oct 8, 2017

Reduced issue:

import core.stdc.stdio;

int dtor_cnt = 0;
struct S57
{
    int v;
    this(int n){ v = n; printf("S.ctor v=%d\n", v); }
    ~this(){ ++dtor_cnt; printf("S.dtor v=%d\n", v); }
    bool opCast(T:bool)(){ printf("S.cast v=%d\n", v); return true; }
}
S57 f(int n){ return S57(n); }

void main()
{
    dtor_cnt = 0;
    if (auto s = (f(1), f(2), f(10)))
    {
        assert(dtor_cnt == 2);
    }
    else assert(0);
    assert(dtor_cnt == 3);
}

The result of f(2) isn't destructed at all (both assertions fail). That test is from 2011.

From -vv:

CommaExp::toElem: (S57 s = (S57 __tmpfordtor67 = f(1);) , __tmpfordtor67 , f(2) , s = f(10);) , s.opCast() @ S57

Edit: A merge regression, I overlooked a line. ;)

@rainers
Copy link
Contributor

rainers commented Oct 8, 2017

The testpdb failure happens, because

  • LDC seems to strip the module name from type names, so "object.Object" becomes "Object". While LDC is consistent with mangling for Object, doing it for every class will cause name conflicts in the debug info.

  • object.Object has no fields. dmd emits information about methods while LDC does not, so it seems empty. This should be no problem, so the test should be switched to another class with fields, e.g. Exception.

@rainers
Copy link
Contributor

rainers commented Oct 8, 2017

BTW: I just noticed that char is now encoded as an unsigned 8-bit integer by LDC. This breaks detection of strings in mago. Was this a deliberate change or something that happened in LLVM? IIRC CodeView has an explicite type for char to distingish from signed 8-bit integer.

@kinke
Copy link
Member Author

kinke commented Oct 8, 2017

Thanks Rainer.

di.d on Win64 with LLVM 5:

class MyClass {
    int a;
    bool myMethod() { return true; }
}

void main() {
    auto str = "this is a string";
    auto c = new MyClass();
}

Wrt. class names:

!14 = !DIDerivedType(tag: DW_TAG_pointer_type, name: "di.MyClass", baseType: !15, size: 64, align: 64)
!15 = !DICompositeType(tag: DW_TAG_structure_type, name: "MyClass", file: !3, line: 1, baseType: !16, size: 192, align: 64, elements: !18, identifier: "C2di7MyClass")
!16 = !DICompositeType(tag: DW_TAG_structure_type, name: "Object", file: !17, line: 89, size: 128, align: 64, elements: !4, identifier: "C6Object")

So we use the fully qualified name for the DI pointer type name (which VS doesn't display, it shows MyClass*) and the base name for the actual DI aggregate types. Sounds definitely wrong.

Wrt. strings, that's got to be an accidental regression:

!37 = !DIBasicType(name: "immutable(char)", size: 8, encoding: DW_ATE_unsigned_char)

@rainers
Copy link
Contributor

rainers commented Oct 8, 2017

So we use the fully qualified name for the DI pointer type name (which VS doesn't display, it shows MyClass*) and the base name for the actual DI aggregate types. Sounds definitely wrong.

AFAICT a pointer type doesn't have a name in CV, I guess it is just discarded.

@kinke
Copy link
Member Author

kinke commented Oct 8, 2017

Wrt. strings, that's got to be an accidental regression

Hmm, not so easy. DW_ATE_UTF still doesn't seem supported (the string then doesn't even show up in VS anymore), and switching between DW_ATE_signed_char and DW_ATE_unsigned_char makes no real difference for the VS debugger. Note that this is the DI encoding of a single char; maybe there's an option to annotate a DI pointer type as string.

@rainers
Copy link
Contributor

rainers commented Oct 8, 2017

Wrt. strings, that's got to be an accidental regression

I took a short look: With the VS C expression evaluator, there is a string type that displays reasonably. With mago, it becomes immutable(ubyte)[].
It's like this for LDC 1.1.0 aswell, so no regression. I suspect the mago engine is looking through typedefs...

@rainers
Copy link
Contributor

rainers commented Oct 8, 2017

I suspect the mago engine is looking through typedefs...

It rebuilt the type inserting "immutable", but that kept the ubyte. I've added a workaround in mago, now. LDC also emits char16/char32 for wstring and dstring which are now supported. So don't worry about the string debug info anymore.

@kinke
Copy link
Member Author

kinke commented Oct 8, 2017

I opened a PR for the runnable/testpdb / general DI issues: #2363

ddmd/func.d has seen a major change wrt. in/out contracts, which are now
generated after inferring the function return type in semantic3.

See dlang/dmd@831552d2047d.
@kinke kinke changed the title Merge 2.076.0 front-end and stdlibs Merge 2.076.1 front-end and stdlibs Oct 10, 2017
Conflicts:
	runtime/phobos
@joakim-noah
Copy link
Contributor

All good on Android/ARM, all the same tests pass as master once I disabled building the new core.stdc.assert_, which I guess is only used by the -betterC pull #2365. I'll look into adding Bionic to that file.

@kinke
Copy link
Member Author

kinke commented Oct 22, 2017

Nope, #2365 doesn't make use of that druntime file at all (edit: and neither does DMD afaict).

@joakim-noah
Copy link
Contributor

joakim-noah commented Oct 22, 2017

OK, in that case, can you add a CMake line to this pull taking it out of the list of D files to compile? It's a declaration-only file anyway, nothing to compile. I suppose it currently warns porters that their OS is not included, but seems pointless to enforce that if it's only opt-in anyway.

@kinke
Copy link
Member Author

kinke commented Oct 22, 2017

Well the compiler uses its own hardcoded set of C assert function signatures, so knowing the one for Android/Bionic would be good in order to make -betterC work properly on Android as well (right know #2365 assumes the normal Linux/glibc mangle+signature). And then adding it to that file too would be a no-brainer.

@kinke kinke merged commit d53bf3e into master Oct 30, 2017
@kinke kinke deleted the merge-2.076 branch October 30, 2017 19:53
@joakim-noah
Copy link
Contributor

joakim-noah commented Nov 13, 2017

Seeing a strange linker error when trying to build the tests for some phobos modules with the latest 1.6 beta release, that I'm not seeing with the 1.5 release:

./ldc2-1.6.0-beta1-linux-x86_64/bin/ldc2 ldc2-1.6.0-beta1-linux-x86_64/import/std/stdio.d -main -unittest

/home/joakim/ldc2-1.6.0-beta1-linux-x86_64/bin/../lib/libphobos2-ldc.a(stdio.o):(.data._D3std5stdio12__ModuleInfoZ+0x0): multiple definition of `_D3std5stdio12__ModuleInfoZ'

stdio.o:(.data._D3std5stdio12__ModuleInfoZ+0x0): first defined here  /usr/bin/ld: Warning: size of symbol `_D3std5stdio12__ModuleInfoZ' changed from 2576 in stdio.o to 40 in /home/joakim/ldc2-1.6.0-beta1-linux-x86_64/bin/../lib/libphobos2-ldc.a(stdio.o)
collect2: error: ld returned 1 exit status
Error: /usr/bin/gcc failed with status: 1

Not sure what's going on, as the symbol appears to be there in both object files in ldc 1.5 also, but I'm only getting an error with the 1.6 beta.

@kinke
Copy link
Member Author

kinke commented Nov 13, 2017

I seem to recall seeing in IR that the ModuleInfos aren't emitted as COMDATs anymore. I thought it'd make no difference as there's one per module. When compiling a stdlib unittest module and linking it against druntime/Phobos, I'd have assumed that only the unittest module is pulled in for linking, but at least in your case it seems to pull in both.

@joakim-noah
Copy link
Contributor

joakim-noah commented Nov 13, 2017

I just tried it with dmd 2.075-7 on linux/x64, cannot reproduce the error with any of the three latest versions, so seems only related to the latest 1.6 beta.

@kinke
Copy link
Member Author

kinke commented Nov 13, 2017

What's the linker command line? Does -lphobos2-ldc come before the actual object file? I don't understand why it would pull in the std.stdio object from the library if that's the main module being compiled (with additional unittests).

Yesterday I linked similar manual unittests (std.random FWIW) without any issues on armhf; that was with the 2.077 branch / the EH PR, but I don't think that makes a difference. No time to check it now.

@joakim-noah
Copy link
Contributor

The linker command is basically identical for 1.5 and 1.6 beta, with stdio.o coming first:

/usr/bin/gcc stdio.o -o stdio -L/home/joakim/ldc2-1.6.0-beta1-linux-x86_64/bin/../lib -L/home/joakim/ldc2-1.6.0-beta1-linux-x86_64/bin/../lib32 -Xlinker --no-warn-search-mismatch -lphobos2-ldc -ldruntime-ldc -Wl,--gc-sections -lrt -ldl -lpthread -lm -m64

/usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0/collect2 -plugin /usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0/liblto_plugin.so -plugin-opt=/usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0/lto-wrapper -plugin-opt=-fresolution=/tmp/ccm2lBpI.res -plugin-opt=-pass-through=-lgcc -plugin-opt=-pass-through=-lgcc_s -plugin-opt=-pass-through=-lc -plugin-opt=-pass-through=-lgcc -plugin-opt=-pass-through=-lgcc_s --build-id --eh-frame-hdr --hash-style=gnu -m elf_x86_64 -dynamic-linker /lib64/ld-linux-x86-64.so.2 -pie -o stdio /usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../lib/Scrt1.o /usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../lib/crti.o /usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0/crtbeginS.o -L/home/joakim/ldc2-1.6.0-beta1-linux-x86_64/bin/../lib -L/home/joakim/ldc2-1.6.0-beta1-linux-x86_64/bin/../lib32 -L/usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0 -L/usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../lib -L/lib/../lib -L/usr/lib/../lib -L/usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0/../../.. stdio.o --no-warn-search-mismatch -lphobos2-ldc -ldruntime-ldc --gc-sections -lrt -ldl -lpthread -lm -lgcc --as-needed -lgcc_s --no-as-needed -lc -lgcc --as-needed -lgcc_s --no-as-needed /usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0/crtendS.o /usr/lib/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../lib/crtn.o

If I try to run the gcc command that ldc 1.5 puts out but with the stdio.o generated by the 1.6 beta, ie linking the 1.6 test object file against the 1.5 stdlib, I still get this error, suggesting the issue is with the 1.6 codegen.

I didn't see this issue with std.random before noting it here either, which is why I said "some phobos modules" initially. I also hit this error with std.utf back then. Trying a handful of random modules now, I only hit it with std.variant, so the fact that it only hits certain modules is doubly strange.

@kinke
Copy link
Member Author

kinke commented Nov 14, 2017

Should be fixed by #2409.
Edit: And I'll simply blindly blame template culling for pulling in the library object. ;)

@joakim-noah
Copy link
Contributor

Yep, that fixes it, confirmed by reverting only that commit and comparing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants