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

Always use 0 for offsets of lib / extern union members #13305

Merged

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Apr 11, 2023

Fixes #9744. See #13205 (comment). Removes the workaround in #7335.

pointerof doesn't have the same issue. Also it seems interpreter specs were broken for a while and fixed here, but I am not 100% sure.

@will @biqqles @zhangkaizhao If possible, please check whether this PR affects #13205, #12589, and #11934 (comment) respectively

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:debugger topic:compiler:codegen labels Apr 11, 2023
@HertzDevil
Copy link
Contributor Author

HertzDevil commented Apr 11, 2023

It seems the requirement for current_debug_file was dropped in LLVM 13: llvm/llvm-project@ad60802

src/llvm/target_data.cr Show resolved Hide resolved
@straight-shoota straight-shoota added this to the 1.8.0 milestone Apr 12, 2023
@will
Copy link
Contributor

will commented Apr 12, 2023

This does look like it fixes it. I think? I wasn't able to easily get ahold of llvm 10.0.0 (the version the darwin-universal has) to try, and it seems like llvm 10.0.1 also fixes it. So it's a little unsatisfying to not be able to build with 10.0.0 to fully verify.

bash-3.2$ ~/Downloads/crystal-1.7.3-1/bin/crystal version
Crystal 1.7.3 [d61a01e18] (2023-03-07)

LLVM: 10.0.0
Default target: aarch64-apple-darwin
bash-3.2$ ~/Downloads/crystal-1.7.3-1/bin/crystal build --debug test.cr
Assertion failed: (Idx < NumElements && "Invalid element idx!"), function getElementOffset, file /var/cache/omnibus/src/llvm/llvm-10.0.0.src/include/llvm/IR/DataLayout.h, line 608.
/Users/will/Downloads/crystal-1.7.3-1/bin/crystal: line 102: 51671 Abort trap: 6           "$INSTALL_DIR/embedded/bin/crystal" "$@"
bash-3.2$ nix run .#crystal_173_llvm10 -- version
Crystal 1.7.3 [d61a01] (1970-01-01)

LLVM: 10.0.1
Default target: aarch64-apple-darwin
bash-3.2$ nix run .#crystal_173_llvm10 -- build --debug test.cr
bash-3.2$ nix run .#crystal_patch_llvm10 -- version
Crystal 1.8.0-dev [da38e4] (1970-01-01)

LLVM: 10.0.1
Default target: aarch64-apple-darwin
bash-3.2$ nix run .#crystal_patch_llvm10 -- build --debug test.cr
bash-3.2$ nix run .#crystal_patch_llvm15 -- version
Crystal 1.8.0-dev [da38e4] (1970-01-01)

LLVM: 15.0.7
Default target: aarch64-apple-darwin
bash-3.2$ nix run .#crystal_patch_llvm15 -- build --debug test.cr
bash-3.2$ nix shell .#crystal_patch_llvm10

test.cr is from your comment on my bug report

lib Foo
  union Bar
    x : Int32
    y : Int16
    z : Int8
  end
end

offsetof(Foo::Bar, @x) # => 0
offsetof(Foo::Bar, @y) # => 110689
offsetof(Foo::Bar, @z)

but it behaves the same as the crunchydata/bridge-cli project with respect to 10.0.1 also seeming to fix the issue, and only failing with the pre-compiled release on github.

flake I used to try all the llvm versions and patches

{
  inputs = {
    flake-utils.url = "github:numtide/flake-utils";

    nixpkgs-crystal = {
      url = "github:will/nixpkgs-crystal";
      inputs.nixpkgs.follows = "nixpkgs";
    };
  };
  description = "crystal tests";

  outputs = { self, nixpkgs, nixpkgs-crystal, flake-utils }:
    let
      systems = (builtins.attrNames nixpkgs-crystal.outputs.packages);
    in
    flake-utils.lib.eachSystem systems (system:
      let
        pkgs = nixpkgs.legacyPackages.${system};

        base = nixpkgs-crystal.packages.${system}.crystal;
        release_src = pkgs.fetchFromGitHub {
          owner = "crystal-lang";
          repo = "crystal";
          rev = "d61a01e185baf84046efce50832a97f097ba0a2f";
          hash = "sha256-ULhLGHRIZbsKhaMvNhc+W74BwNgfEjHcMnVNApWY+EE=";
        };
        patch_src = pkgs.fetchFromGitHub {
          owner = "HertzDevil";
          repo = "crystal";
          rev = "da38e42a66df7bbc2e6e5a6265f4c1f9cf3f2980";
          hash = "sha256-q4nr8qMMRDxJyJ6eURdcIsRi5dsj6ZQ18V4eXMjrr0U=";
        };

      in
      {
        packages = {
          crystal_173_llvm10 = base.override {
            llvm = pkgs.llvm_10;
            src = release_src;
          };

          crystal_patch_llvm10 = base.override {
            llvm = pkgs.llvm_10;
            src = patch_src;
          };

          crystal_patch_llvm15 = base.override {
            llvm = pkgs.llvm_15;
            src = patch_src;
          };
        };
      }
    );
}

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Apr 12, 2023

In that case your 10.0.1 might have had assertions disabled

@biqqles
Copy link

biqqles commented Apr 13, 2023

Thanks for the ping, this looks very promising!

Unfortunately I can't test properly for Solus as since compiling that branch (the latest release 1.7.3 works fine) results in the bundled libs not being found?

x86_64-solus-linux-g++ -c -mtune=generic -march=x86-64 -g2 -O2 -pipe -fPIC -fno-plt -D_FORTIFY_SOURCE=2 -fstack-protector-strong --param=ssp-buffer-size=32 -fasynchronous-unwind-tables -ftree-vectorize -feliminate-unused-debug-types -Wall -Wno-error -Wp,-D_REENTRANT  -o src/llvm/ext/llvm_ext.o src/llvm/ext/llvm_ext.cc -I/usr/include -std=c++14   -fno-exceptions -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
CRYSTAL_CONFIG_BUILD_COMMIT="72a7a7da0" CRYSTAL_CONFIG_PATH=lib:/usr/lib64/crystal/src SOURCE_DATE_EPOCH="1681393377"  CRYSTAL_CONFIG_LIBRARY_PATH='$ORIGIN/../lib/crystal' ./bin/crystal build -D strict_multi_assign -D preview_overload_order --release --link-flags="-Wl,--copy-dt-needed-entries -Wl,-O1 -Wl,-z,relro -Wl,-z,now -Wl,-z,max-page-size=0x1000 -Wl,-Bsymbolic-functions -Wl,--sort-common" -Dwithout_interpreter  -o .build/crystal src/compiler/crystal.cr -D without_openssl -D without_zlib -D use_pcre2
Showing last frame. Use --error-trace for full trace.

In src/compiler/crystal/tools/doc/generator.cr:1:1

 1 | require "markd"
     ^
Error: can't find file 'markd'

If you're trying to require a shard:
- Did you remember to run `shards install`?
- Did you make sure you're running the compiler in the same directory as your shard.yml?
make: *** [Makefile:213: .build/crystal] Error 1

reply also cannot be found if I try to build with interpreter support. I can see that libs/ is populated properly via treeing it so no idea why this is suddenly happening after 1.7.3. Can bisect later if necessary.

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Apr 13, 2023

Does $(./bin/crystal env CRYSTAL_PATH) include the lib directory in this repository? Also apparently this was due to #13040

@biqqles
Copy link

biqqles commented Apr 13, 2023

Awesome, thank you for saving me some time, adding the lib to CRYSTAL_PATH indeed resolved it.

So the compiler specs are fixed! But unfortunately the interpreter is still not usable ☹️

$ crystal i
Crystal interpreter 1.8.0-dev [72a7a7da0] (2023-04-13).
EXPERIMENTAL SOFTWARE: if you find a bug, please consider opening an issue in
https://github.com/crystal-lang/crystal/issues/new/
crystal: /home/build/YPKG/root/llvm/build/llvm-project-14.0.6.src/llvm/include/llvm/Support/Casting.h:269: typename cast_retty<X, Y *>::ret_type llvm::cast(Y *) [X = llvm::StructType, Y = llvm::Type]: Assertion `isa<X>(Val) && "cast<Ty>() argument of incompatible type!"' failed.
Aborted (core dumped)

(This is a different assertion error to, I am assuming, what this PR fixes.)

@straight-shoota straight-shoota merged commit 5dc9424 into crystal-lang:master Apr 13, 2023
@biqqles
Copy link

biqqles commented Apr 13, 2023

@HertzDevil or @straight-shoota -- do I need to open an issue for the interpreter assertion failure or should I refocus my original report?

@HertzDevil HertzDevil deleted the bug/extern-union-offsetof branch April 14, 2023 01:59
@straight-shoota
Copy link
Member

This interpreter issue is tracked in #13317

joebonrichie pushed a commit to solus-packages/crystal that referenced this pull request Aug 14, 2023
Summary:
[Changelog](https://github.com/crystal-lang/crystal/blob/1.8.0/CHANGELOG.md)

This build contains fixes for two LLVM assertion failures ([1](crystal-lang/crystal#13305), [2](crystal-lang/crystal#13319)) that previously prevented the compiler specs from passing and interpreter support from working respectively. The latter of these will be released in 1.9.0 but I include it here as a patch.

Test Plan: Tested compilation of simple program and `crystal i`.

Reviewers: #triage_team, Staudey

Reviewed By: #triage_team, Staudey

Subscribers: Staudey

Differential Revision: https://dev.getsol.us/D13954
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:codegen topic:compiler:debugger
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler bug: invalid Int32 when calling offsetof(lib union)
4 participants