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

stage2: error return traces #11501

Merged
merged 6 commits into from
May 17, 2022
Merged

Conversation

Vexu
Copy link
Member

@Vexu Vexu commented Apr 22, 2022

Currently disabled on all targets because LLVM is being a dick and I'm too tired to fight it.

$ zig2 test test/behavior.zig -I test -fLLVM    -target aarch64-linux
LLVM Emit Object... 
inlinable function call in a function with debug info must have a !dbg location
  call fastcc void @builtin.returnError(%builtin.StackTrace* %0) #13
inlinable function call in a function with debug info must have a !dbg location
  call fastcc void @builtin.returnError(%builtin.StackTrace* %0) #13
inlinable function call in a function with debug info must have a !dbg location
  call fastcc void @builtin.returnError(%builtin.StackTrace* %0) #13
inlinable function call in a function with debug info must have a !dbg location
  call fastcc void @builtin.returnError(%builtin.StackTrace* %0) #13

Not that it was that useful anyways:

$ zig2 run a.zig
error: Foo
???:?:?: 0x20b078 in ??? (???)
???:?:?: 0x20b145 in ??? (???)

I tried to do as much as possible in Sema without touching parameters, if that is also desired it should be done alongside the begin_call/end_call change.

Closes #11259
Closes #11084

@Vexu Vexu force-pushed the stage2-err-return-trace branch 2 times, most recently from c4b393b to 61d25c6 Compare April 23, 2022 12:35
@andrewrk andrewrk self-requested a review April 23, 2022 20:05
@schmee
Copy link
Contributor

schmee commented Apr 26, 2022

@Vexu does this PR solve #1923 as well?

@Vexu
Copy link
Member Author

Vexu commented Apr 26, 2022

@Vexu does this PR solve #1923 as well?

No, currently this just matches stage1.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do a second review on the error return traces implementation, but I want to block the merge solely on the basis of 61d25c6.

build.zig should not assume that there is a config.h file in an arbitrary directory called "build". The intended way that it works currently is if you run zig build from inside a build directory that has already been used via the cmake build process, then it will find config.h from there. On my machine, I have many build directories:

[nix-shell:~/dev/zig]$ ls build
build/              build-debug/        build-llvm13-debug/ build-release/      build-llvm14/

They each have a different config.h, and arbitrarily choosing build/config.h is incorrect.

As for the silent default, what I would expect is for it to search for LLVM via your system default prefix. I see that it is hitting these lines:

            // Here we are -Denable-llvm but no cmake integration.
            try addStaticLlvmOptionsToExe(exe);
            try addStaticLlvmOptionsToExe(test_stage2);

Looking at the implementation of these functions, I don't think there is anything to change. These are perfectly reasonable defaults for the case when one choosing to build from source without the cmake bootstrapping step. Indeed, this is the path we use for the CI as well as zig-bootstrap.

My suggestion for your workflow is to follow the official building from source instructions and execute the zig build commands from inside the appropriate cmake build directory, or to continue to use the -Dconfig_h option.

@Vexu
Copy link
Member Author

Vexu commented May 2, 2022

Looking at the implementation of these functions, I don't think there is anything to change.

They do need a ZIG_PREFER_CLANG_CPP_DYLIB equivalent so I guess I'll implement that instead.

@andrewrk
Copy link
Member

andrewrk commented May 2, 2022

Will that actually improve anyone's workflow? For your case, you have a build folder from cmake, correct? Then the correct way to build is using that same information, using the cwd method or with -Dconfig_h. Package maintainers building a zig package will do the same thing. People using zig-bootstrap will use static libs and never have a use case for dynamic clang.

@Vexu
Copy link
Member Author

Vexu commented May 2, 2022

Well, not currently at least, but if we end up ditching the bootstrap compiler of cleaned output of stage2 with cbe only then there would be no config.h?

@andrewrk
Copy link
Member

andrewrk commented May 2, 2022

Agreed - at that point the logic for integrating with system LLVM, Clang, LLD will become the business of build.zig.

@Vexu Vexu force-pushed the stage2-err-return-trace branch from 61d25c6 to 821c374 Compare May 3, 2022 07:25
@andrewrk andrewrk self-assigned this May 16, 2022
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I like how you were able to move some of the functions into userland.

I'm sorry for taking so long to review this. It's ready to be merged as far as I'm concerned.

@@ -1791,6 +1811,27 @@ pub const Object = struct {
"", // unique id
);
}

fn getStackTraceType(o: *Object) Type {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a little fishy. I think it's OK for now, but before this function gets copy+pasted into the other backends, let's look into moving this logic into Sema. Perhaps for a Module in which error return tracing is enabled, it can create a root reference to this type and store it directly in Module.

@andrewrk andrewrk force-pushed the stage2-err-return-trace branch from 821c374 to 0a7f3be Compare May 17, 2022 00:43
@andrewrk
Copy link
Member

andrewrk commented May 17, 2022

I hope you don't mind - I took the liberty to rebase against master branch and force push. I'm running the behavior tests locally and plan to press the merge button if they pass. Actually I think I'll wait for the CI since it has been rebased against many commits.

@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. frontend Tokenization, parsing, AstGen, Sema, and Liveness. labels May 17, 2022
@andrewrk andrewrk removed their assignment May 17, 2022
@andrewrk andrewrk merged commit df74c45 into ziglang:master May 17, 2022
@Vexu Vexu deleted the stage2-err-return-trace branch May 17, 2022 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. frontend Tokenization, parsing, AstGen, Sema, and Liveness.
Projects
None yet
3 participants