In D142963#4093094, @tmatheson wrote:

It looks like this has these odd behaviours:

• -target-feature +v9.3a -target-feature -v9.2a will remove v9.2a but add all the dependent features of both v9.3a and v9.2a. It also doesn't remove v9.3a which implies v9.2a.
• -target-feature -v9.2a -target-feature +v9.3a will add the v9.2a back, and also add all the dependent features of both v9.3a and v9.2a.
• -target-feature +v9.3a -target-feature -v9.3a will remove v9.3a but add all the dependent features of v9.3a.

This seems like a bit of a minefield. Is there even a use case for negative architectural features?

In D139102#3963388, @DavidSpickett wrote:

Compiler infrastructure should not assume the existence of a Linux distribution. The portable way is simple and can easily be supported. One should be able to install $prefix/lib/libzstd.a and $prefix/include/zstd.h, then have that prefix searched as part of the standard CMAKE_PREFIX_PATH.

In D119380#3310110, @mgorny wrote:

Sounds like you're trying hard to make me step down from contributing to LLVM anymore.

Sounds good to me

My goal is to use the same method for compiling llvm, clang, and lld as package maintainers, so that we don't get any "surprises" when someone packages our application using system llvm, lld, clang packages. I don't really care which way is the correct way to compile the software but the existence of multiple ways to do it creates problems for me.

Why stop there? Just keep it simple and fail the build.

Andrew Kelley <andrew@ziglang.org>

Thank you for having a look, Adrian! I would appreciate if you or someone else could commit for me since I do not have write access.

Confirmed, I have tested this change and it solves the problem.

I am still in the process of testing this change.

ping?

I was not aware of any reliance on a system assembler within LLVM. My previous understanding was that one of the features of LLVM is to provide assemblers for all of these architectures, which is useful for cross compiling.

LGTM

LGTM

That makes sense 👍

This will be nice to prevent bugs in the future. Should it be behind #ifndef NDEBUG though? Presumably if assertions are disabled then this bug would be expected to be undefined behavior and avoid the overhead of the thread local variable, is that right?

I tested this and confirm that it solves the bug (which I reported).

I confirm this solves the issue.

Thanks Hans. I'll need you to do it on my behalf as I do not have commit privileges.

In D56297#1345884, @aprantl wrote:

Is the problem that it is a SubroutineType, but not a function *pointer*?
Generally, if the Verifier accepts it we ought to not crash, so this seems to be okay, but it might be worth asking whether this is really what you want your frontend to be creating since subroutinetypes don't have a size, etc.

In D54667#1341372, @rnk wrote:

In that case, you might want to add the DINode::FlagStaticMember flag to all your methods. If a DISubprogram lists a DICompositeType as its scope or is used as an element of the composite type, it gets treated as a C++-style method where the first argument is assumed to be 'this'. That's a very C++-centric way to do things, so it might be nicer if we explicitly marked C++ instance methods, but that's the way things are today and changing it would require thinking about backwards compatibility. >_>

The benefit of explicitly marking everything as a "static" method for zig is that if the first argument happens to be a pointer type it won't end up looking like an instance method.

In D54667#1340985, @rnk wrote:

We can change the code to avoid the crash, but what debug info did you want clang to emit? It seems like the best way to describe StructWithNoFields_add is as a static method of the class StructWithNoFields, since it doesn't appear to have a 'this' parameter.

I think this patch broke Zig's tests:

Added Rui's test case. Thank you for helping me with that. Please note that I do not have commit privileges and so someone will have to merge this on my behalf.

In D51265#1215060, @efriedma wrote:

What is this change actually solving? Even if the typedef is actually redundant, it shouldn't cause a compiler error: redundant typedefs are allowed in the latest versions of the C and C++ standards. (And even if you were targeting an earlier standard, we normally suppress warnings in system headers.)

Addressed review feedback

Thanks. Can you do the commit? I do not have commit access.

Ping

Ping. Is it time yet? Looks like the stated blockers are resolved now.

I confirm that this fixes https://bugs.llvm.org/show_bug.cgi?id=36991

Would llvm interact with grow_memory or memory_size instructions? Seems like that would be the frontend or standard library's job. It's not in LLVM's business to interact with the "OS" by allocating memory.

Is there anything stopping this from landing in svn?

Never mind these changes. I've redesigned how coroutines will work in Zig and they no longer depend on these changes.

Hans, thank you for reviewing. I do not have commit access; would you mind committing it for me, in whatever order you prefer?

rebase

I would have selected a repository but the instructions explicitly say not to:

Leave the Repository field blank.

I'll be careful next time, thanks. I used the web interface. Arcanist seemed more involved, but if that is preferable I can do it next time.

Thank you, I will CC llvm-commits next time. I followed these instructions: https://llvm.org/docs/Phabricator.html

I would also like to request that this is backported to the release_60 branch.

Which we prefer, maximum IO performance or a reliable error handling?

• Can we have this even without tests?
  • Can we have this in 5.0.0?