LGTM, but since we both work at Arm, let's wait a week for other folks to chime in if they have any objections.

ping^4

LGTM. Let's wait for someone else to approve (or commit after holidays if no one objects).

Does this change break OSS-Fuzz? Did anyone try?

I did not know that these tests are supposed to interact with an external component. Sorry about that. I'll wait for approval from the libc++ approvers in my future patches.

Fix formatting

Use TableGen patterns for ISel when possible

LGTM

clang shall either die due an assert in debug [...]

I think I have already mentioned the reasons why I don't think this is acceptable:

• it would break tests, as they are usually run with enabled assertions (and since this diagnostic is quite easy to test, it should be tested)
• it would make the code harder to debug (i.e. if someone finds another problem related to large includes, they would first need to fix the failing assertions before they could start debugging their problem)

ping^3

I don't know signal frame but I think the purpose of that assert is to check unw_step return value > 0 from main if that is a signal frame. Adding another frame will make that assert always be true, regardless of signal frame, thus it is not testing signal frame anymore.

That might not be the case. unw_is_signal_frame works after the stepping out of the signal frame, i.e. we need to call unw_step anyway. @saugustine is the author of the test and he approved my patch, so presumably the test should not depend on whether the signal_frame function is main() or not.

ping^2

In D70397#1753767, @steven_wu wrote:

In this patch, add another stack frame completely defeat the purpose of assert(unw_step(&cursor) > 0);.

Is the DWARF unwinder supposed to be always able to step out of main()?

In D70183#1751813, @dnsampaio wrote:

Git diff 979ae80af7ec49624b932954d22cb91900f17121 did not send a test as well. Feel free to send me a reasonable sized reproducer, the one I have is about 36MB. Don't think it will be that well received.

This essentially means that if there is a problem related to this change, say something goes wrong after return FileID(); it would be impossible to debug until the corresponding assertions are fixed. Also the patch lacks tests (and adding a test is impossible because it would fail in debug builds).

In D70183#1751552, @dnsampaio wrote:

Yes. It does return a non-valid FileID, and in builds without assert you get the expected error message.

ping

The motivation is to be able to make source locations' underlying type configurable. Richard Smith suggested that this might be feasible: http://lists.llvm.org/pipermail/cfe-dev/2019-October/063515.html
So, the first step is to get rid of getRawEncoding where possible. I think this would make the code cleaner anyway.

Changed getRawEncoding -> getHashValue in Sema.h

In D68342#1710616, @lebedev.ri wrote:

In D68342#1710600, @miyuki wrote:

@lebedev.ri, do you agree that all address spaces should be treated the same way as address space 0 (i.e. no signed overflow)?

I wouldn't be surprised if that isn't so, i don't think it's really documented what normal assumptions do and don't apply to non-0-address-spaces.

OK, let's stick to what is documented for GEP (and the status quo about objects larger than half of the address space).

Don't treat non-zero address space specially

@lebedev.ri, do you agree that all address spaces should be treated the same way as address space 0 (i.e. no signed overflow)?

Added the NSW flag. This discussion thread also suggests that signed overflows should not occur in the inbounds GEPs: https://lists.llvm.org/pipermail/llvm-dev/2017-November/118914.html

ping

So clang is perfectly correct here.

Yes, Clang is correct. EmitGEPOffset is doing the wrong thing.
nuw is incorrect because negative offsets are allowed. nsw would also be incorrect because of the quote you mentioned before:

I also thought that "in bounds address of an allocated object" has something to do with the type used in the GEP instruction, but that's not how Clang interprets it.
E.g. for the following code

int read(int *buf) {
  buf -= 2;
  return *buf;
}

It generates the following:

define dso_local i32 @_Z4readPi(i32* nocapture readonly %buf) local_unnamed_addr #0 {
entry:
  %add.ptr = getelementptr inbounds i32, i32* %buf, i64 -2
  %0 = load i32, i32* %add.ptr, align 4, !tbaa !2
  ret i32 %0
}

I do not understand this point, could you elaborate please?

In D68342#1691617, @lebedev.ri wrote:

I'm not fully convinced this is correct, as per
https://llvm.org/docs/LangRef.html#getelementptr-instruction

If the inbounds keyword is present, the result value of the getelementptr is a poison value
if the base pointer is not an in bounds address of an allocated object, or if any of the
addresses that would be formed by successive addition of the offsets implied by the indices
to the base address with infinitely precise signed arithmetic are not an in bounds address
of that allocated object. <...>

I also thought that "in bounds address of an allocated object" has something to do with the type used in the GEP instruction, but that's not how Clang interprets it.
E.g. for the following code

Added from __future__ import print_function

In D68138#1686487, @greened wrote:

What happens when this is run with python 2 in the path? Should this explicitly run via python3?

Could you please look into https://bugs.llvm.org/show_bug.cgi?id=43206 ?

Hi. Could you please look into https://bugs.llvm.org/show_bug.cgi?id=43183 (it seems to be related)