In D52164#1237188, @hfinkel wrote:

disable this fold when comparing noalias ptr with null

But it's not just comparing to a nullptr constant, it's also comparing to another SSA value that, dynamically, is null.

Yes, we only fold if it is not used. :)

int f(int n) {

int *x = (int *)malloc(n);
free(x);
int *y =  (int *)malloc(n);
return (y == x) ;

}

Ok.. so maybe

• disable this fold when comparing noalias ptr with null
• disable also when cmp' rhs and lhs are both noalias ptrs

Yes, I will add more tests. Suggestions welcome.

"High bits are zeros" and "result is always non negative" are good candidatates to be catched here. Thanks for detailed review, Richard. Now I will look on it.

MSVC warns for every case in the test file I uploaded here. https://godbolt.org/z/jiMFp6

Hm, isUnsignedIntegerType seems to ignore uintN_t types?

Added fixit hints

I think we can emit fixit hint always, not even for -u32 case :)

1. Error -> Warning

15-20 times / 10 000 lines. I checked and first two are bugs, I don't track other warnings yet.

@RKSimon are fine to rewrite X & -X to X & ((~X)+1) in bmiintrin.h?

The first is not broken, but it is now detected by this new check. I think the second form is more explicit and better.

/home/xbolva00/LLVM/build/lib/clang/8.0.0/include/bmiintrin.h:312:16: error: unary minus operator applied to type 'unsigned long long', result value is still unsigned

return __X & -__X;

ping

Is this ready or any blockers?

Looks awesome!

Yeah, it would be super useful if Clang can add const to all places where possible :) love this work, great!

Where here, I would love to have a fixint hints for unknown functions.

In rL342052#287591, @xbolva00 wrote:

In rL342052#287584, @RKSimon wrote:

In rL342052#287579, @xbolva00 wrote:

Thanks for fix

Btw, sorry for hijacking, but GCC trunk reports another warning in ArrayRef:

/home/davidbolvansky/trunk/llvm/include/llvm/ADT/ArrayRef.h:102:37: warning: initializing ‘llvm::ArrayRef<int>::Data’ from ‘std::initializer_list<int>::begin’ does not extend the lifetime of the underlying array [-Winit-list-lifetime]
102 | : Data(Vec.begin() == Vec.end() ? (T*)nullptr : Vec.begin()),

Should we worry?

Probably! Has it turned up on a buildbot?

I don't think so since -Winit-list-lifetime is added in GCC 9.0 (as well as -Wdeprecated-copy; I saw some deprecated copy warnings too :/). I will share the list of warnings on llvm bugzilla tomorrow.

Adjusted regexes in two more tests

Seems fine for trunk now..

So basically the hardest problem is to "find a way to get stdio from a process and feed to to LLDB so it shows up when neither --tty nor --no-stdio is set" ..

In D51966#1231943, @clayborg wrote:

We should obey the --tty option here and set CREATE_NEW_CONSOLE if it is set no?

In rL342052#287584, @RKSimon wrote:

In rL342052#287579, @xbolva00 wrote:

Thanks for fix

Btw, sorry for hijacking, but GCC trunk reports another warning in ArrayRef:

/home/davidbolvansky/trunk/llvm/include/llvm/ADT/ArrayRef.h:102:37: warning: initializing ‘llvm::ArrayRef<int>::Data’ from ‘std::initializer_list<int>::begin’ does not extend the lifetime of the underlying array [-Winit-list-lifetime]
102 | : Data(Vec.begin() == Vec.end() ? (T*)nullptr : Vec.begin()),

Should we worry?

Probably! Has it turned up on a buildbot?

Thanks for fix

Check https://bugs.llvm.org/show_bug.cgi?id=38912

• Fixed failing test

In D51966#1231533, @labath wrote:

I think the issue here is that on windows we don't have the ability(*) to pipe stdio through lldb, so if you create a process without a console, you will not be able to use see it's output or pass it some input (probably fine for gui apps, but bad for console ones). This means your approach will also make things inconsistent with other platforms, only in a different way. (I don't really have an opinion which one is better/worse.)

(*) I think there is a way to emulate the unix pty behavior on windows, but it is not exactly straight-forward. It involves creating a hidden console and then polling that for changes/keypresses (I don't know the details, but that's how someone once explained it to me).

Please check also https://reviews.llvm.org/rL339521 :)

Convert paths to platform native paths

In D51847#1230766, @zturner wrote:

Alright I see it. The reason I'm asking is because after your change you're now printing main.o: main.c ../include/lib/test.h. But I wonder if you should instead be printing main.o: main.c ..\include\lib\test.h. It seems like paths that we show to the user should be in the native path style.

Please see PR38877.

Makes sense. Please rebase before you commit this patch.

Thanks, looks good.

Can we rework GetLanguageForExpr? Or atleast rename it.

In D51847#1228927, @zturner wrote:

What prints this? How do you exercise this codepath?

Remove some dead code

Maybe worth to put this PR33222's fix to release branch too? (+ https://reviews.llvm.org/rL341778)

Tests?

Please check https://bugs.llvm.org/show_bug.cgi?id=38807

In D51615#1226500, @stella.stamenova wrote:

In D51615#1226494, @xbolva00 wrote:

In D51615#1226445, @stella.stamenova wrote:

In D51615#1226396, @xbolva00 wrote:

Bot sure how to fix tests since this is somehow hard to problematic.

I think the fix for this is simply to not make the call to SetUseColor here. Is there a reason for the call? It looks like the check for "dumb" terminal and "GetIsTerminalWithColors" might have to come after setting the ANSI escape codes if setting the codes impacts those two cases.

We may disable "use color" setting thanks to GetIsTerminalWithColors but then we realize that we can "enable it" via SetConsoleMode for newer Win 10, so we need to call it I think. The reverse order does not help us either.. I think.

Sounds like GetIsTerminalWithColors might have to be updated to include the new logic as well. Would you rather commit a fix or revert the change and then commit a new one?

In D51615#1226445, @stella.stamenova wrote:

In D51615#1226396, @xbolva00 wrote:

Bot sure how to fix tests since this is somehow hard to problematic.

I think the fix for this is simply to not make the call to SetUseColor here. Is there a reason for the call? It looks like the check for "dumb" terminal and "GetIsTerminalWithColors" might have to come after setting the ANSI escape codes if setting the codes impacts those two cases.

Bot sure how to fix tests since this is somehow hard to problematic.

We should not abandon it. It seems we can optimize this case

is this fixed in 7.0 release branch too?

• Check for _WIN32

• use llvm::sys::Process::UseANSIEscapeCodes

@teemperor please take a look again :)

So what is proposed solution?

In D50233#1223705, @efriedma wrote:

But that also implies that you have to emit an inline implementation since you might not control what the implementation of memcmp supplied by libc does?

Assuming there isn't a data race, the libc implementation of memcmp can't actually do anything undefined. In assembly, "uninitialized" data always has some fixed value.

---

Given the problems here, we probably want to move this transform to the ExpandMemCmp pass, and only transform when we'll expand the compare inline.

In D50233#1223686, @adamreichold wrote:

In D50233#1223683, @efriedma wrote:

Does LLVM's implementation somehow guarantee that no later optimization pass comes along, sees the program reading uninitialized memory and declares UB?

In LLVM IR, reading uninitialized memory isn't itself UB; it's only UB if you perform certain operations on the result. Granted, a naive LLVM IR implementation of memcmp might involve operations which lead to UB.

But that also implies that you have to emit an inline implementation since you might not control what the implementation of memcmp supplied by libc does?

In D50233#1223654, @eugenis wrote:

GCC does not have MemorySanitizer.